Skip to content

fix: Update output: printer to avoid range(a, b, 0) #138

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

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

Kakalinn
Copy link
Contributor

Description

If names has length <n you get a ValueError.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Issue Number

I decided to push this instead of an issue.

Code Compatibility

  • I have performed a self-review of my code

Code Performance and Testing

  • I have added tests that prove my fix is effective or that my feature works
  • I ran the complete Pytest test suite locally, and they pass

Dependencies

  • I have ensured that the code is still pip-installable after the changes and runs
  • I have tested that new dependencies themselves are pip-installable.

Documentation

  • My code follows the style guidelines of this project
  • I have updated the documentation and docstrings to reflect the changes
  • I have added comments to my code, particularly in hard-to-understand areas

Additional Notes

I assume the 4 is there so the output isn't spammed. Maybe it should be configurable.

If `names` has length <n you get a ValueError.
I assume the 4 is there so the output isn't spammed.
Maybe it should be configurable.
@FussyDuck
Copy link

FussyDuck commented Feb 13, 2025

CLA assistant check
All committers have signed the CLA.

@ecmwf ecmwf deleted a comment from github-actions bot Feb 14, 2025
@HCookie
Copy link
Member

HCookie commented Feb 14, 2025

Thanks for identifying and proposing a fix for this.
I'm curious in what use case you got this? Having less then 4 fields seems odd.

Once you rename the PR to follow conventional titles and sign the CLA, we can approve and merge.

@Kakalinn Kakalinn changed the title Update output: printer to avoid range(a, b, 0) feat: Update output: printer to avoid range(a, b, 0) Feb 14, 2025
@Kakalinn Kakalinn changed the title feat: Update output: printer to avoid range(a, b, 0) fix: Update output: printer to avoid range(a, b, 0) Feb 14, 2025
@Kakalinn
Copy link
Contributor Author

Haha, odd indeed.
I just wanted to get a tiny model trough all the stages on my computer.

Copy link

Caution

This pull request contains changes to GitHub workflows!
Proceed with caution and if not sure, contact your GitHub admin.

@HCookie
Copy link
Member

HCookie commented Feb 17, 2025

I'm not sure what the bot is doing here, but if you accept the CLA we can look at merging soon.

@HCookie HCookie self-assigned this Feb 17, 2025
@HCookie HCookie self-requested a review February 17, 2025 11:10
@Kakalinn
Copy link
Contributor Author

I thought I had already, sorry.
It should be done now.

@HCookie HCookie merged commit 7cb2f0d into ecmwf:main Feb 17, 2025
17 of 20 checks passed
@HCookie
Copy link
Member

HCookie commented Feb 17, 2025

Thanks for the contribution.
It has been merged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants