Add lint warning for tools with cores and no mem
#140
Conversation
Pull Request Test Coverage Report for Build 11805014496Details
💛 - Coveralls |
There was a problem hiding this comment.
This looks great, thanks @natefoo! I think it should finally take away all those spurious warnings.
This might be a bit hairy to merge with: https://github.com/galaxyproject/total-perspective-vortex/pull/136/files but that's on me - I still need to enhance the linter to use that PR, and check types.
tpv/core/util.py
Outdated
| def load_yaml_from_url_or_path(url_or_path: str): | ||
| yaml = ruamel.yaml.YAML(typ='safe') | ||
| def load_yaml_from_url_or_path(url_or_path: str, round_trip: bool = False): | ||
| typ = "rt" if round_trip else "safe" |
There was a problem hiding this comment.
Can we simplify this to make it roundtrip always? Is there a disadvantage?
There was a problem hiding this comment.
With safe the data loads as plain Python objects (e.g. dicts and lists) and changing the loader to rt makes them CommentedMap, CommentedList, etc. Since only the linter needs round trip it this felt safer to me not knowing the code very well, but if you don't think it would cause problems I'm happy to change it to always load round trip.
There was a problem hiding this comment.
Regarding a disadvantage, if all the data are copied from the loader Entity objects (and not by copying the ref to the loader object representation) then it's probably ok. If not, it could potentially increase memory usage slightly and I am not certain everything would merge the same way without digging into ruamel.yaml.
There was a problem hiding this comment.
If I remember correctly, the current implementation is unlikely to preserve ruamel objects when copying. However, the current implementation will be obsolete once we merge this: #136
I'm guessing pydantic won't preserve ruamel either?
There was a problem hiding this comment.
Ok, I changed it to always use roundtrip. The formatter will need some extra work though to preserve comments through to writing.
There was a problem hiding this comment.
The round trip formatter is implemented as well, this should be good to go.
Related discussion in galaxyproject/tpv-shared-database#73.
I looked a bit at adding#noqasupport but reading comments and figuring out the line they are associated with seemed a bit tricky. But if it's something we want I could work on that. Alternatively,tpv lint --ignore=...to simply ignore an entire class of warnings would be easy to implement, but then it's all-or-nothing.I went ahead and added
# noqaand command linetpv lint --ignore=...support.# noqacomments apply to the whole entity they are contained within and can either be without a specific code (in which case all warnings are ignored) or can specify codes as a comma-separated list like# noqa: T101, T102, .... Inline# noqacomments are not supported.IMO you'd probably not want these warnings on your local TPV config, but we probably do want them on the shared DB.