Remove dead attributes in application #673
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## smartsim-refactor #673 +/- ##
=====================================================
+ Coverage 40.45% 42.72% +2.26%
=====================================================
Files 110 109 -1
Lines 7326 6980 -346
=====================================================
+ Hits 2964 2982 +18
+ Misses 4362 3998 -364
|
MattToast
left a comment
MattToast
left a comment
There was a problem hiding this comment.
Looks great! Just some minor clean up here and there!!
I want to discuss some of those lingering TODOs with the group before I sign off and we merge anything, but otherwise this is looking like a much more refined model to me! Should be ready to go once we get consensus on those last few items!
mellis13
left a comment
mellis13
left a comment
There was a problem hiding this comment.
Overall looks great! Just minor questions/comments.
smartsim/entity/application.py
Outdated
|
|
||
| @property | ||
| def exe_args(self) -> t.Sequence[str]: | ||
| """Return an immutable list of attached executable arguments. |
There was a problem hiding this comment.
is the returned value really immutable?
There was a problem hiding this comment.
At runtime, not technically, but the type hint here states that a sequence is return which implicitly promotes immutability. If we do drop the "immutable" wording from the docstring, I'm going to also recommend changing the return type to list[str] or at the very least MutableSequence[str]
There was a problem hiding this comment.
Yeah, I guess I was thinking that _build_exe_args returns a List which is mutable so that part was out of sync.
Do we want to be specific and risk API breaks later by changing to list[str] or generic to limit breaks later (i.e. MutableSequence[str])
There was a problem hiding this comment.
changed to mutablesequence[str]
MattToast
left a comment
MattToast
left a comment
There was a problem hiding this comment.
A few more minor nits to consider while you sort out the conflict, but otherwise looks about ready to go to me!
MattToast
left a comment
MattToast
left a comment
There was a problem hiding this comment.
One last question, but nothing worth holding approval up over. LGTM! Thanks for all the hard work cleaning up this interface!!
4 participants
No description provided.