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

Keep _constraint and _pretty_constraint of Dependency synchronized when calling set_constraint() #214

Merged

Conversation

radoering
Copy link
Member

Resolves: python-poetry/poetry#4589

  • Added tests for changed code.
  • Updated documentation for changed code.

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Is there a compelling reason we can't remove the internal property and rely on the getter to call str()? I'm not super familiar with the code here, but this just seems to be much more error-prone than computing the value on demand.

@radoering radoering force-pushed the keep-pretty-constraint-in-sync branch from bb469a8 to a4dc4eb Compare November 21, 2021 06:09
@radoering
Copy link
Member Author

The only reason I can imagine (without having examined it) is performance.

@neersighted
Copy link
Member

Let's go ahead and drop the property then -- I think that implementation will be much more obviously correct and will not allow things to get out of sync in the future.

@radoering
Copy link
Member Author

After dropping the property, I had to adapt some tests because the constraint style changed. With the property pretty_constraint was determined before parsing the constraint, now it is determined after parsing the constraint.

@neersighted
Copy link
Member

After dropping the property, I had to adapt some tests because the constraint style changed. With the property pretty_constraint was determined before parsing the constraint, now it is determined after parsing the constraint.

Gah, looks like it's a pretty leaky abstraction overall. I'm inclined to just roll back to your first changeset and plan to refactor this later.

@radoering
Copy link
Member Author

No problem. In any case, less risky.

@neersighted neersighted merged commit af08f1c into python-poetry:master Nov 21, 2021
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.

[poetry-core] Inconsistent state of Dependency after calling set_constraint()
2 participants