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

Fail as intended for to short lists #42

Merged
merged 1 commit into from
Jun 5, 2023
Merged

Conversation

niklassiemer
Copy link
Member

In recent pyiron_contrib dependabot PRs (e.g. pyiron/pyiron_contrib#696), the number of arguments was wrong (different issue) and the action failed (as intended). However, not with the nice massage we intended (see below). This hopefully fixes this.

Run pyiron/actions/update-env-files@main
  with:
    conda-yml: .ci_support/environment.yml
Run pip install pyyaml
  pip install pyyaml
  shell: /usr/bin/bash -l {0}
Defaulting to user installation because normal site-packages is not writeable
Requirement already satisfied: pyyaml in /usr/lib/python[3](https://github.com/pyiron/pyiron_contrib/actions/runs/5111616226/jobs/9188774199#step:3:3)/dist-packages (5.[4](https://github.com/pyiron/pyiron_contrib/actions/runs/5111616226/jobs/9188774199#step:3:5).1)
Run python $GITHUB_ACTION_PATH/../.support/update_environment.py Bump pyiron-base from 0.[5](https://github.com/pyiron/pyiron_contrib/actions/runs/5111616226/jobs/9188774199#step:3:6).3[8](https://github.com/pyiron/pyiron_contrib/actions/runs/5111616226/jobs/9188774199#step:3:10) to 0.5.3[9](https://github.com/pyiron/pyiron_contrib/actions/runs/5111616226/jobs/9188774199#step:3:11)  $GITHUB_ACTION_PATH/../.support/pypi_vs_conda_names.json
  python $GITHUB_ACTION_PATH/../.support/update_environment.py Bump pyiron-base from 0.5.38 to 0.5.39  $GITHUB_ACTION_PATH/../.support/pypi_vs_conda_names.json
  shell: /usr/bin/bash -l {0}
Traceback (most recent call last):
  File "/home/runner/work/_actions/pyiron/actions/main/update-env-files/../.support/update_environment.py", line [10](https://github.com/pyiron/pyiron_contrib/actions/runs/5111616226/jobs/9188774199#step:3:12)3, in <module>
    if not matches_dependabot_pattern_for_yaml_update(sys.argv):
  File "/home/runner/work/_actions/pyiron/actions/main/update-env-files/../.support/update_environment.py", line 100, in matches_dependabot_pattern_for_yaml_update
    return pattern_is_bump_from_to(list_) or pattern_is_update_requirement_from_to(list_)
  File "/home/runner/work/_actions/pyiron/actions/main/update-env-files/../.support/update_environment.py", line 94, in pattern_is_update_requirement_from_to
    ends_in_yml(list_[8])
IndexError: list index out of range
Error: Process completed with exit code 1.

@liamhuber
Copy link
Member

Looks fine, but do we want to double down and raise an error with a custom and extremely descriptive message in the false case?

@niklassiemer
Copy link
Member Author

I am not sure, does it not already raise the descriptive error now? I mean we only know that the input was not as expected and that is what the other massage says as well?

@liamhuber
Copy link
Member

Sure. It's some sort of string parsing here though, right? We could be like "we expected a string formatted like blah blah but got the string {string}" is more detail that might be helpful? I don't feel strongly here though

@niklassiemer
Copy link
Member Author

Sure. It's some sort of string parsing here though, right? We could be like "we expected a string formatted like blah blah but got the string {string}" is more detail that might be helpful? I don't feel strongly here though

This is exactly what you did if these tests fail 😄 I just modified the tests for special patterns and if the len does not fit, a False is the expected answer for a check function.

Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

Super!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants