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

Consistently use none to indicate non-set function ids #466

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lauraengelhardt
Copy link
Contributor

@lauraengelhardt lauraengelhardt commented Feb 28, 2025

Description and Context

  • Replaces 0s by nones for all input function ids (FUNCTs).
  • Remove all checks for non-positive function ids before calling function_by_id.

If you now use 0, -1, or any other invalid value in the input file to indicate you do not want to supply a FUNCT, an error is thrown by function_by_id.

I'll supply a python script I used for the replacements if needed:
replace_0_by_none_for_FUNCTs.txt
You need to fix the extension, github doesn't allow to upload *.py files.

Related Issues and Pull Requests

--

@lauraengelhardt lauraengelhardt added team: input breaking change Pull requests that require manual user actions after merging labels Feb 28, 2025
@lauraengelhardt lauraengelhardt self-assigned this Feb 28, 2025
@lauraengelhardt
Copy link
Contributor Author

Let's see what tests fail. I'll let you know once this is ready.

@lauraengelhardt lauraengelhardt force-pushed the use-none-for-nonexistent-functions branch 2 times, most recently from fa65738 to 2c4a800 Compare February 28, 2025 18:31
@lauraengelhardt lauraengelhardt force-pushed the use-none-for-nonexistent-functions branch from 6f76526 to 5d2e2e3 Compare March 1, 2025 21:54
@lauraengelhardt lauraengelhardt changed the title WIP: Consistently use none to indicate non-set function ids Consistently use none to indicate non-set function ids Mar 2, 2025
@lauraengelhardt
Copy link
Contributor Author

This PR is ready.

@sebproell
Copy link
Member

@lauraengelhardt Thanks! This makes things much more consistent. I just wonder how big the fallout from this change is going to be. As far as I can tell, this will break every input file. Since we had a lot of breaking changes anyway, we might as well throw this one in as well. On the other hand, we should really start versioning of the input file (#74) and allow to perform these adaptations automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that require manual user actions after merging team: input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants