Improve readability of host/platform eval code #53
Conversation
| # remove at: | ||
| # Cylc8.x | ||
| if not command: | ||
| if not eval_str: | ||
| return 'localhost' | ||
|
|
||
| # Host selection command: $(command) or `command` |
There was a problem hiding this comment.
This docstring needs changing - it's no longer just the host, and if it's the platform the pattern shouldn't match `command` .
Because of the way you've generalized the function I'd be happy just deleting it.
| return command | ||
| return os.path.expandvars(eval_str) | ||
|
|
||
| # BACK COMPAT: references to "host" |
There was a problem hiding this comment.
Make this part of the eval_host docstring.
Suggest noting that when we remove the back compat layer we re-merge eval_platform and _subshell_eval, else there will be a non purposeful layer of abstraction?
There was a problem hiding this comment.
Make this part of the
eval_hostdocstring.
I think it's fine to leave as a comment? It doesn't tell developers how to use the function which is what docstring is primarily for.
Suggest noting that when we remove the back compat layer we re-merge
eval_platformand_subshell_eval, else there will be a non purposeful layer of abstraction?
I think that should be fairly self-evident when the time comes
|
wxtim
force-pushed
the
fix_localhost_platform_matching
branch
from
9df1a9f to
83484ef
Compare
MetRonnie
force-pushed
the
subshell-eval
branch
from
e8b9802 to
12ce3ef
Compare
|
@oliver-sanders can you have a quick look at this as part of cylc#5343? |
|
Looks the same. |
Does that mean it's good to go (with tests added) or it's not any more readable? |
@oliver-sanders - Do you mean it's Good to merge IYO |
Answer was both! @wxtim Let me know if you want me to add tests else feel free to undraft + merge |
…gex_remote_tidy_fail * upstream/8.1.x: Update cylc/flow/scripts/message.py Upload coverage to Codecov in separate job (cylc#5459) upgrade cylc message internal help with details of severity levels Update tests/functional/platforms/10-do-not-host-check-platforms.t Fix flake8-comprehensions C419 Don't use any([i for i in iterable]) use any(i for i in iterable). It's more efficient because we don't have to expand the entire thing. Improve readability of host/platform eval code (#53) small changlog error fix update comment on localhost check and add test for case localhost4.localhost42 undo mistake clarification of nomenclature Avoid running host check on platform names - this doesn't make any sense.
cylc#5343
Not added any tests yet