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

IGNITE-24187 Fix code style issues in SystemDistributedConfigurationPropertyHolder #5027

Merged
merged 3 commits into from
Jan 21, 2025

Conversation

kgusakov
Copy link
Contributor

@kgusakov kgusakov commented Jan 12, 2025

Copy link
Contributor

@tkalkirill tkalkirill left a comment

Choose a reason for hiding this comment

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

The ticket description states what will be done:

CompletableFuture.completedFuture(null) must be replace nullCompletedFuture

I didn't see these changes.

@kgusakov
Copy link
Contributor Author

The ticket description states what will be done:

CompletableFuture.completedFuture(null) must be replace nullCompletedFuture

I didn't see these changes.

Ticket description was fixed. Still think that the additional dependency from the core module creates more potential issues in future, than fixes now.

Copy link
Contributor

@tkalkirill tkalkirill left a comment

Choose a reason for hiding this comment

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

Ticket description was fixed. Still think that the additional dependency from the core module creates more potential issues in future, than fixes now.

Please describe what potential issues there might be.

@kgusakov
Copy link
Contributor Author

Ticket description was fixed. Still think that the additional dependency from the core module creates more potential issues in future, than fixes now.

Please describe what potential issues there might be.

Every additional dependency makes it difficult to change the project structure in future. On the other hand the only plus is a nullCompletedFuture() vs completedFuture(null) - it is the same number of symbols though and nullCompletedFuture() will never encapsulate any logic I suppose.

But on the other hand: core module is a special type of module, looks like we will never want to create any deps in it, which can create a problem cases. Will fix it here, ok.

@sk0x50 sk0x50 merged commit b64d2b1 into apache:main Jan 21, 2025
1 check passed
@sk0x50 sk0x50 deleted the ignite-24187 branch January 21, 2025 10:13
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.

4 participants