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

Guarantee initial order for decorators to sort #1154

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

Sgitario
Copy link
Collaborator

@Sgitario Sgitario commented Feb 9, 2023

Plus, add some test coverage with basic scenarios.

Copy link
Member

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

The compareTo seems awkward so it either needs to change or some comments explaining why.
The TreeSet part is also not clear to me.

core/src/main/java/io/dekorate/utils/TopologicalSort.java Outdated Show resolved Hide resolved
@@ -32,7 +33,8 @@ private TopologicalSort() {
*/
public static List<Decorator> sortDecorators(Collection<Decorator> items) {
Collection<Node> nodes = adaptToNodes(items);
return sortNodes(nodes);
// Order the nodes by the decorator class to guarantee the initial order.
return sortNodes(new TreeSet<>(nodes));
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me what you mean by initial order.

Copy link
Collaborator Author

@Sgitario Sgitario Feb 21, 2023

Choose a reason for hiding this comment

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

The idea is to get a determinist order if we always get the same input. For example, if our input is a set of three decorators with no dependencies on each other, the final result order of these three decorators might vary in different executions.

The idea I had to address this situation is to introduce an initial order based on the decorator class name instead of the decorator order insertion. Therefore, and following the above example, the three decorators will be ordered by the decorator name and hence the final result order will now always be the same.

Copy link
Member

Choose a reason for hiding this comment

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

The idea is to get a determinist order if we always get the same input. For example, if our input is a set of three decorators with no dependencies on each other, the final result order of these three decorators might vary in different executions.

The order of decorators should not be important to us as long as they are topologically sorted. Even if it's possible the get different results that both satisfy the constraints, the end result (the generated json/yaml files) should be deterministic. If the end result is not deterministic, then we probably have not properly expressed those constraints and we should focus on that.

TDLR: I don't mind having the decorators in a predictable order however, we shouldn't have to, are we sure we are dealing with the issue at the right level?

Copy link
Member

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

LGTM as long as we fix the toString thingy.

Plus, add some test coverage with basic scenarios.
@sonarcloud
Copy link

sonarcloud bot commented Feb 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@Sgitario Sgitario merged commit 5f73d25 into dekorateio:main Feb 22, 2023
@Sgitario Sgitario deleted the changes branch February 22, 2023 19:54
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.

2 participants