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

[22.01] Revert startswith of tool parameter options #14440

Merged
merged 3 commits into from
Aug 18, 2022

Conversation

qiagu
Copy link
Contributor

@qiagu qiagu commented Aug 12, 2022

I think the previous change broke the startswith.
ping @bernt-matthias

(Please replace this header with a description of your pull request. Please include BOTH what you did and why you made the changes. The "why" may simply be citing a relevant Galaxy issue.)
(If fixing a bug, please add any relevant error or traceback)
(For UI components, it is recommended to include screenshots or screencasts)

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

@github-actions github-actions bot added this to the 22.09 milestone Aug 12, 2022
@bernt-matthias
Copy link
Contributor

Hi @qiagu. The previous change indeed changed the behavior.. but it brought it in line with the documented behavior.

See details in #14332

Or am I wrong?

@bernt-matthias
Copy link
Contributor

You are right @qiagu

The wrong docs is due to cc23b0e that was included in a PR crafted by you and me :)

So the code needs to be reverted, but docs and tests need fixes.

@bernt-matthias
Copy link
Contributor

Can you target the 22.01 branch?

@qiagu
Copy link
Contributor Author

qiagu commented Aug 12, 2022

I guess we want to use the lines with startswith instead of skipping them. Sorry for documentation issue.

@bernt-matthias
Copy link
Contributor

Sorry for documentation issue.

No worries. The commit was by me :)

@bgruening
Copy link
Member

FAILED test/functional/test_toolbox_pytest.py::test_tool[select_from_csvdataset_test_1]

I think the test failure looks legitimate.

@anuprulez
Copy link
Member

I tested this change on this branch: release_22.05_europe (https://github.com/usegalaxy-eu/galaxy) and unfortunately I could not make it work with hyperparameter search tool with option startswith="@". But, when I removed not from this change and used startswith="*", the tunable params in a list populated inside hyperparameter search tool gets correctly loaded. I am not sure if the option startswith is used to include or exclude text starting with the specified parameter. The tabular file has both, * and @ present in the beginning of the file.

@bernt-matthias
Copy link
Contributor

startswith="@" should keep only the lines that start with @

@qiagu
Copy link
Contributor Author

qiagu commented Aug 17, 2022

@bernt-matthias I wonder whether you would like to do the rest. I imagine the remaining work includes fixing tests and docs. Thanks.

@bernt-matthias bernt-matthias changed the base branch from dev to release_22.01 August 18, 2022 08:49
@bernt-matthias bernt-matthias changed the base branch from release_22.01 to dev August 18, 2022 08:50
@bernt-matthias bernt-matthias changed the base branch from dev to release_22.01 August 18, 2022 08:55
@bernt-matthias bernt-matthias removed this from the 22.09 milestone Aug 18, 2022
@bernt-matthias bernt-matthias changed the title Revert startswith of tool parameter options [22.01] Revert startswith of tool parameter options Aug 18, 2022
@github-actions github-actions bot added this to the 22.05 milestone Aug 18, 2022
@mvdbeek mvdbeek merged commit 30dc1bf into galaxyproject:release_22.01 Aug 18, 2022
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.

5 participants