Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add PythonEnvInfo-related helpers. #14051
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
Add PythonEnvInfo-related helpers. #14051
Changes from all commits
1126834
0d22681
4468e74
94137c8
eea499b
7a2fdfc
ca2ec73
af6c7c5
e737607
51c6dee
2ecd537
9cd6c7d
6b50973
ad8f195
2b70a1c
7b19538
eb746f9
20c46da
9d970f5
ce53e54
3d85fa7
e47322b
f97bb31
1edb76d
fd5e37b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I don't think we neeed
old
at all. We normally use this to search for the environment which is to be replaced byupdate
, but we can also useupdate
to search for the environment to replace.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason to keep it is because the object may not be the same as the one at the index. The one at the index is the one to use as "old", but it can be helpful to have the
event.old
as well, especially when debugging. So I'd like to keep it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I don't understand your point, but we're using
areSameEnvironments
to look for matching environments, so we do not need to do exact object comparison to replace the one at the index. The only purpose forevent.old
was to search for the old environment which was fired and replace it.Also, in the resolver, I found out that it's a bit hard to maintain the exact "old" environment to be used for firing in certain circumstances.
Yaa maybe, although if that's the only reason I think we should remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
event.old
only for debugging purposes?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing
old
would take some work, so I'm fine with keeping it for now. I was just curious about the above question^^There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could certainly be used for debugging. However, I can imagine it being used for other things too.