Removed an argument to concretize that was there only for unit tests#12213
Conversation
Instead of using an argument to deactivate displaying a tree within tests, we'll just use a fixture and monkey patch where appropriate.
lib/spack/spack/environment.py
Outdated
| status_fn=spack.spec.Spec.install_status, | ||
| hashlen=7, hashes=True) | ||
| ) | ||
| sys.stdout.write(_tree_to_display(concrete)) |
There was a problem hiding this comment.
IMO this is slightly less clear than the original logic (IMO fixtures are harder to understand than parameters). I do agree that in general, it is bad practice to switch off logic inside of a function. In this particular case, the function is mixing datastructure maintenance and UI concern, so I felt this was harmless. I think it could definitely be refactored to be cleaner; IMO this could return the specs that are newly-concretized and then the command logic could print them (and the calls in the tests could just discard them).
Does that seem reasonable?
There was a problem hiding this comment.
In this particular case, the function is mixing datastructure maintenance and UI concern, so I felt this was harmless.
I agree that this function as it is has possibly too many responsibilities. To explain why I did this refactor: in #11372 concretize delegates to two different private functions, depending on whether we are concretizing the specs together or not. The argument _display had to be pushed down to those functions so instead of doing that I removed the argument completely.
Does that seem reasonable?
It seems reasonable to me, with one minor caveat. Spec.concretize returns None and modifies the calling object state while Spec.concretized returns the concretized spec in a new object. Doing the proposed change in Environment we'll be having a method that is named like the former in Spec but behaves more like the latter.
@becker33 @scheibelp In the end I am fine with either this approach or the one suggested by Peter above. Let me know how to proceed.
There was a problem hiding this comment.
I'm fine with this change in service to #11372 - if you moved these changes to that PR I would accept the changes there; IMO it's hard to justify this separate from #11372 though. The way #12213 (comment) was phrased made me think this was more of a style change.
If you prefer to address this separately, I would prefer it be done as I requested in #12213 (comment).
It seems reasonable to me, with one minor caveat. Spec.concretize returns None and modifies the calling object state while Spec.concretized returns the concretized spec in a new object. Doing the proposed change in Environment we'll be having a method that is named like the former in Spec but behaves more like the latter.
That's fine with me: concretize can mean something different for an Environment vs. a Spec.
There was a problem hiding this comment.
I'm fine with this change in service to #11372 - if you moved these changes to that PR I would accept the changes there; IMO it's hard to justify this separate from #11372 though.
Not a big deal but:
- These changes were cherry-picked from Spack environments can concretize specs together #11372 and moved here as I think they don't belong in that PR
- It's also a style issue as, at least afaik, argument to functions with a leading underscore are not idiomatic in Python + having functionality in code that is needed only for unit tests is also not a good practice
In the end I wouldn't have changed this part if I didn't have to resolve a conflict, but once I made the modification I thought it was clearer to submit it separately. I'll take a further step and try to move the UI part to the command.
There was a problem hiding this comment.
@scheibelp Let me know if the latest version is what you had in mind, or if further modifications are needed.
The UI part of Environment.concretize has been moved into its own function. The "client" code is now responsible to make an additional call to display the specs, if it needs to. This permits to remove fixtures from unit tests and the additional argument to the concretize method.
|
Thanks! I appreciate your flexibility on this! |
|
No worries, thanks for the review! |
Environment.concretize returns newly-concretized specs rather than printing them; as a result, the _display argument is removed from Environment.concretize (originally only used to avoid printing specs during unit testing). Command logic which invokes Environment.concretize prints these explicitly.
4 participants
While rebasing #11372 I noticed that the
Environment.concretizemethod changed signature in 0715b51:spack/lib/spack/spack/environment.py
Line 823 in 0715b51
as a new argument
_displaygot added. This argument seems to be used only in unit tests to switch off part of the code. To avoid:this PR extracts the logic to display specs into its own function. Client code is then responsible to make an additional call if it needs to display the result of concretization - and unit tests can simply skip that call.