Skip to content
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

Ruff and flake8 fixes #16884

Merged
merged 3 commits into from
Oct 19, 2023
Merged

Ruff and flake8 fixes #16884

merged 3 commits into from
Oct 19, 2023

Conversation

nsoranzo
Copy link
Member

Extracted from the Python 3.12 support PR.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@@ -1435,13 +1435,12 @@ def _workflow_to_dict_export(self, trans, stored=None, workflow=None, internal=F
if name:
input_dicts.append({"name": name, "description": annotation_str})
for name, val in step_state.items():
input_type = type(val)
if input_type == RuntimeValue:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks so intentional to me - like we did this at some point to avoid ConnectedValue? I can't find evidence of that in the blame logs though. I think it is fine to keep this as is and just revert if there are problems. If this is actually the check we want - I think ideally we should be using is_runtime_value, again not a necessary thing though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the insight!
By looking at the implementation of is_runtime_value:

def is_runtime_value(value):
    return isinstance(value, RuntimeValue) or (
        isinstance(value, dict) and value.get("__class__") in ["RuntimeValue", "ConnectedValue"]
    )

it's clear that it would not avoid ConnectedValue either.
I don't see a reason to avoid it here, but if there is one, then we would need to revert this change or use if isinstance(val, RuntimeValue) and not isinstance(val, ConnectedValue):

@nsoranzo nsoranzo merged commit 245b4dc into galaxyproject:dev Oct 19, 2023
42 checks passed
@nsoranzo nsoranzo deleted the ruff_flake8_fixes branch October 19, 2023 14:42
@galaxyproject galaxyproject deleted a comment from github-actions bot Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants