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

[23.2] Yaml nested assertions: fix parsing #17641

Merged

Conversation

bernt-matthias
Copy link
Contributor

Seems that the recursive call was forgotten.

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.

were forgotten to be implemented
@bernt-matthias bernt-matthias changed the title [23.2] Yaml nested assertions: fix [23.2] Yaml nested assertions: fix parsing Mar 8, 2024
@github-actions github-actions bot added this to the 23.2 milestone Mar 8, 2024
@bernt-matthias bernt-matthias force-pushed the yaml-nested-assertions branch from 57aeecf to 73ced4e Compare March 8, 2024 11:51
@bernt-matthias bernt-matthias force-pushed the yaml-nested-assertions branch from 73ced4e to df7c2af Compare March 8, 2024 11:52
"""
- has_archive_member:
path: ".*"
children:
Copy link
Member

@mvdbeek mvdbeek Mar 8, 2024

Choose a reason for hiding this comment

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

I guess that's what your comment about asserts was about ? It does feel more consistent for that to be

asserts:
  - has_text:
    text: thing

?

Copy link
Contributor Author

@bernt-matthias bernt-matthias Mar 8, 2024

Choose a reason for hiding this comment

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

I used the syntax from the other examples in the file

I'm still struggling with yaml, but it seems that

yaml.load("""
asserts:
  - has_text:
    text: thing
"""
)

gives `{'asserts': [{'has_text': None, 'text': 'thing'}]}`

while

```python
yaml.load("""
asserts:
  - has_text:
      text: thing
"""
)

gives {'asserts': [{'has_text': {'text': 'thing'}}]}. Not sure if both versions are parsed at the moment.

Not sure if the former even works.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, typing without checking, it should be

asserts:
  - has_text:
      text: thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is what I meant with my comment at the planemo issue

Not sure, but easy to implement I guess. But I can clearly live with the current state. The only question would be if it would simplify anything if we use asserts?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, then we can re-use models and there's no question what you have to use IMO - it's always asserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then remove children completely, or leave it as falll back? I guess since it seems that this was never used, we may just change it?

Copy link
Member

Choose a reason for hiding this comment

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

Leave it as a fallback but don't document it would be my suggestion ?

@mvdbeek mvdbeek merged commit b73d9e2 into galaxyproject:release_23.2 Mar 11, 2024
43 of 46 checks passed
Copy link

This PR was merged without a "kind/" label, please correct.

@bernt-matthias bernt-matthias deleted the yaml-nested-assertions branch March 11, 2024 11:54
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.

2 participants