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

fix(bc): better handling of default values #2004

Merged
merged 12 commits into from
Apr 18, 2024

Conversation

MartinBelthle
Copy link
Contributor

@MartinBelthle MartinBelthle commented Apr 8, 2024

This pull request significantly enhances the management of default values when using the create_binding_constraint and update_binding_constraint commands. It also addresses an issue with updating terms of a binding constraint using the update_binding_constraint command.

Corrections have also been made to the CommandExtractor class, which extracts commands from a base study and a modified study through comparison, and to the CommandReverter class, which reverses commands based on a command history.

The generation of a study from a list of commands has been corrected: now, the generation stops as soon as an error is encountered, making it easier to identify the failing command, as it is necessarily the last one executed.

Copy link
Contributor

@laurent-laporte-pro laurent-laporte-pro left a comment

Choose a reason for hiding this comment

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

I don't know where those __getattribute__ comes from, but it is a bad practice. We should use getattr() function instead.

@MartinBelthle
Copy link
Contributor Author

modify the constraint term to remove optional values

@laurent-laporte-pro laurent-laporte-pro added this to the v2.17 milestone Apr 9, 2024
@laurent-laporte-pro laurent-laporte-pro marked this pull request as draft April 9, 2024 13:29
@MartinBelthle MartinBelthle force-pushed the bugfix/v8.7-binding-constraints-commands branch from 89d87a7 to daad91b Compare April 9, 2024 15:57
@MartinBelthle MartinBelthle marked this pull request as ready for review April 9, 2024 16:00
@MartinBelthle
Copy link
Contributor Author

There's an issue with the removal of constraint terms.

@MartinBelthle MartinBelthle force-pushed the bugfix/v8.7-binding-constraints-commands branch from daad91b to 7198064 Compare April 10, 2024 08:36
@laurent-laporte-pro laurent-laporte-pro force-pushed the bugfix/v8.7-binding-constraints-commands branch from 2810609 to 07b6329 Compare April 10, 2024 16:38
@MartinBelthle
Copy link
Contributor Author

MartinBelthle commented Apr 12, 2024

@laurent-laporte-pro I've checked your commit and I think you should revert it because you missed this part of the code that makes your test fail inside the old match function in create_binding_constraint:

    simple_match = self.name == other.name
    if not equal:
        return simple_match

So IMO we should not touch at this code as it would mean a big refactoring for other commands and we do not want that in this PR

@MartinBelthle MartinBelthle force-pushed the bugfix/v8.7-binding-constraints-commands branch from c8195fc to 84d2b7a Compare April 16, 2024 07:45
@laurent-laporte-pro laurent-laporte-pro force-pushed the bugfix/v8.7-binding-constraints-commands branch from 4922187 to 6f0a2c7 Compare April 18, 2024 08:02
@laurent-laporte-pro laurent-laporte-pro force-pushed the bugfix/v8.7-binding-constraints-commands branch from 6f0a2c7 to e70dae6 Compare April 18, 2024 08:04
@laurent-laporte-pro
Copy link
Contributor

@laurent-laporte-pro I've checked your commit and I think you should revert it because you missed this part of the code that makes your test fail inside the old match function in create_binding_constraint:

    simple_match = self.name == other.name
    if not equal:
        return simple_match

So IMO we should not touch at this code as it would mean a big refactoring for other commands and we do not want that in this PR

I've done a rollback + refactoring. It's OK now.

@laurent-laporte-pro laurent-laporte-pro merged commit e524b13 into dev Apr 18, 2024
6 of 7 checks passed
@laurent-laporte-pro laurent-laporte-pro deleted the bugfix/v8.7-binding-constraints-commands branch April 18, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants