Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: use diamond operator #1405

Merged

Conversation

gregorriegler
Copy link
Contributor

Co-authored-by: Moderne team@moderne.io

Copy link
Contributor

@troshko111 troshko111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

One change request: please revert diamond operator usages where the expression no longer has the type specified on the left hand side (i.e. the type is completely erased from the expression).

Good: Set<String> cnamesSet = new TreeSet<~String~>();
Since the type appears on the left, the rhs is redundant.

Bad:

heartbeatExecutor = new ThreadPoolExecutor(
                    1, clientConfig.getHeartbeatExecutorThreadPoolSize(), 0, TimeUnit.SECONDS,
                    new SynchronousQueue<>(),
                    new ThreadFactoryBuilder()
                            .setNameFormat("DiscoveryClient-HeartbeatExecutor-%d")
                            .setDaemon(true)
this.applications = new ConcurrentLinkedQueue<>();

Here the type does not appear on the lhs, and erasing from the rhs make it impossible to tell what it is.

@gregorriegler gregorriegler force-pushed the refactor/use-diamond-operator-4RNCw branch from 0c84c9b to a4b2f98 Compare June 16, 2021 21:39
@gregorriegler
Copy link
Contributor Author

gregorriegler commented Jun 16, 2021

Ok, I went through the changes and reverted the ones where no type-info was left.

@troshko111 troshko111 merged commit 27a635b into Netflix:master Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants