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

Depracation warning for parameter names not being valid python identifiers #337

Merged
merged 17 commits into from
Dec 20, 2023

Conversation

quaquel
Copy link
Owner

@quaquel quaquel commented Dec 15, 2023

See #274 for details. This just turns the exception into a deprecation warning in preparation for the 2.5 release

@coveralls
Copy link

coveralls commented Dec 15, 2023

Coverage Status

coverage: 79.631% (-0.7%) from 80.335%
when pulling 45ba964 on valid_identifers_deprc
into f3c9674 on master.

@EwoutH
Copy link
Collaborator

EwoutH commented Dec 15, 2023

Globally, looks good!

Ideally, clean up the commits into:

  • The actual code changes
  • The warning added
  • Test changed
  • All the model / example changes (so all the conversions, underscores)
  • Files changed

Otherwise squashing on merge would also be fine. I would just make sure it doesn't cause major conflicts with #274.

@EwoutH EwoutH added this to the 2.5.0 milestone Dec 15, 2023
@quaquel
Copy link
Owner Author

quaquel commented Dec 15, 2023

How would I manage reorganizing the commits because I agree that a clean history would be a convenient.

@EwoutH
Copy link
Collaborator

EwoutH commented Dec 15, 2023

In my process I use GitHub desktop. You can drag and drop commits to reorder them. Then you squash the ones that work on the same feature (for example squashing all the testing commits to one commit). See Reordering commits in GitHub Desktop.

If you're more of a CLI person: 7.6 Git Tools - Rewriting History.

@quaquel quaquel merged commit e030eab into master Dec 20, 2023
20 of 22 checks passed
@quaquel quaquel deleted the valid_identifers_deprc branch December 20, 2023 09:38
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.

3 participants