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

Aggregation_id #222

Merged
merged 7 commits into from
Jul 5, 2022
Merged

Conversation

perolavsvendsen
Copy link
Member

Solving #221

  • Change the API - no longer accept non-string input for aggregation_id
  • Returned aggregation_id is now a string
  • Returned aggregation_id is now independent of sorting (same input give same output, regardless of input sorting)
  • Add some more tests to it

Copy link

@roywilly roywilly left a comment

Choose a reason for hiding this comment

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

ok

@perolavsvendsen perolavsvendsen marked this pull request as ready for review July 1, 2022 06:38
Comment on lines 935 to 937
aggregation_id: Give an explicit ID for the aggregation. If None, or not
provided, an automatic ID based on existing realization uuid will be made.
Default is None which means it will be missing (null) in the metadata.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something is self-contradicting here. What will None actually give?

Copy link
Member Author

Choose a reason for hiding this comment

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

Clarified

stringinput += xuuid

return uuid_from_string(stringinput)
return str(uuid_from_string(stringinput))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps it would be better if the uuid_from_string function do the str() conversion

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@jcrivenaes jcrivenaes left a comment

Choose a reason for hiding this comment

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

Review comments first; otherwise ok

@perolavsvendsen perolavsvendsen merged commit c6655e4 into equinor:main Jul 5, 2022
@perolavsvendsen perolavsvendsen deleted the aggregation_id branch July 5, 2022 07:31
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

3 participants