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.

Sign up for GitHub

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

Jump to bottom

[22.05] fix looks_like_yaml_or_cwl_with_class #15439

Merged
merged 5 commits into from
Jan 30, 2023
Merged

[22.05] fix looks_like_yaml_or_cwl_with_class #15439

merged 5 commits into from
Jan 30, 2023

Conversation

bernt-matthias
Copy link
Contributor

the string class: ... might appear on the first line hence the leading \n was wrong

I guess its better to use ^ and $ and add the MULTILINE flag

Bug has been introduced here: #12604

Suggestions for tests welcome

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.

@bernt-matthias bernt-matthias changed the base branch from dev to release_22.05 January 29, 2023 12:15
@github-actions github-actions bot modified the milestones: 23.1, 23.0 Jan 29, 2023
@bernt-matthias bernt-matthias mentioned this pull request Jan 29, 2023
Merged
@mvdbeek
Copy link
Member

mvdbeek commented Jan 29, 2023

Maybe plug in one of the failing documents as a test

@bernt-matthias bernt-matthias force-pushed the fix-looks-like-yaml-w-class branch from 767891f to e7d7385 Compare January 29, 2023 12:49
@bernt-matthias
add unit test for is_a_yaml_with_class
e7d7385 
as used in planemo
@bernt-matthias
fix looks_like_yaml_or_cwl_with_class
4c4cc2c 
the string `class: ...` might appear on the first line
hence the leading `\n` was wrong

I guess its better to use `^` and `$` and add the MULTILINE flag
@bernt-matthias
Copy link
Contributor Author

OK commited a test first and test/unit/tool_util/test_loader_directory.py::test_is_a_yaml_with_class is failing https://github.com/galaxyproject/galaxy/actions/runs/4036844700/jobs/6939735971

Now, lets push the fix again.

@bernt-matthias @mvdbeek
Test improvements
8ec08f6 
Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>
mvdbeek
mvdbeek approved these changes Jan 30, 2023
View reviewed changes
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Thank you!

@bernt-matthias
Copy link
Contributor Author

Cool. Maybe we can get this and #14572 .. and get another prerelease of 22.06 packages?

@mvdbeek mvdbeek merged commit 7c8d732 into galaxyproject:release_22.05 Jan 30, 2023
@mvdbeek
Copy link
Member

mvdbeek commented Jan 30, 2023

Thank you @bernt-matthias!

@github-actions
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvdbeek mvdbeek mvdbeek approved these changes

Assignees
No one assigned
Labels
area/tool-framework kind/bug
Projects
None yet
Milestone
23.0
Development

Successfully merging this pull request may close these issues.
2 participants
@bernt-matthias @mvdbeek