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

[instruments] Remove CommentID from getInstanceData. #8805

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Jun 21, 2023

The CommentID is not part of the data, it's the foreign key used between the flag table and the instrument table. JSON-based instruments do not have it, and this ensures better consistency between the two so that issues such as #8796 and #8801 will not vary based on instrument type and will be caught sooner.

Resolves part of #8804 (Inconsistency #2)

The CommentID is not part of the data, it's the foreign key used
between the flag table and the instrument table. JSON-based instruments
do not have it, and this ensures better consistency between the two
so that issues such as aces#8796 and aces#8801 will not vary based on instrument
type and will be caught sooner.

Resolves part of aces#8804 (Inconsistency #2)
@driusan driusan added Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) Cleanup PR or issue introducing/requiring at least one clean-up operation Caveat for Existing Projects PR contains changes that might impact the code or accepted practices of current active projects 25.0.0 - Bugs labels Jun 21, 2023
@driusan
Copy link
Collaborator Author

driusan commented Jun 21, 2023

@xlecours assuming this passes the tests can you review it? You were the author of the 2 PRs referenced as inspiration.

Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

Let's see if other tests will break.

@driusan driusan merged commit c2f41af into aces:25.0-release Jun 21, 2023
16 checks passed
@ridz1208 ridz1208 added this to the 25.0.0 milestone Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
25.0.0 - Bugs Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) Caveat for Existing Projects PR contains changes that might impact the code or accepted practices of current active projects Cleanup PR or issue introducing/requiring at least one clean-up operation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants