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

[minor] remove ImportAlarm and deprecator stuff from the API #1458

Closed
wants to merge 5 commits into from

Conversation

liamhuber
Copy link
Member

With a helpful error message if you actually try to use any of them.

This follows up on @jan-janssen's comment, following the path of leaving the objects importable, but raising an error redirecting the user to the new location if they try to use it.

I did my best to ensure that our redirection message will stay consistent with any upstream changes in pyiron_snippets.

In order to seamlessly keep our downstream repositories working, it would be good to merge the following PRs to use pyiron_snippets before releasing these changes on pyiron_base:

For import alarm:

For deprecator stuff:

With a helpful error message if you actually try to use any of them
@liamhuber
Copy link
Member Author

The failures are just the linters complaining about the unused imports -- but they're not unused, they're making sure the stuff we're redirecting people to via a string is actually where we think it is.

@jan-janssen
Copy link
Member

The failures are just the linters complaining about the unused imports -- but they're not unused, they're making sure the stuff we're redirecting people to via a string is actually where we think it is.

Please fix the complains from ruff. I am not so critical about the ones from Codacy.

@jan-janssen jan-janssen marked this pull request as draft June 1, 2024 13:11
@liamhuber
Copy link
Member Author

The failures are just the linters complaining about the unused imports -- but they're not unused, they're making sure the stuff we're redirecting people to via a string is actually where we think it is.

Please fix the complains from ruff. I am not so critical about the ones from Codacy.

The failures from ruff are exactly what I described above -- ruff complains that the imports are unused, but I, as a human and not a bot, know that they are not unused but rather guaranteeing that what I say in my string is valid.

I'm open to an alternative formulation that uses the import in an automated way to generate the string, but, as I also mentioned (in a commit message?) I couldn't figure this out because 'deprecate' is actually just an instance of 'deprecator' and doesn't have that name associated with it in any way, it's just a variable name not an attribute property.

@jan-janssen
Copy link
Member

ruff is a strict requirement

A linter added a comma and pushed us over the edge
Because the snippets have associated code and ruff wants all the imports first
@liamhuber
Copy link
Member Author

Quoted from #1455 (comment)

PRs before the deprecate stuff can be removed from the API

pyiron_atomistics is save because it has an upper bound on pyiron_base. So we are just waiting for pyiron_contrib. I set this pull request to draft until things are cleaned up in pyiron_contrib.

pyiron_continuum is not using upper bounds either, so we should leave this draft until its PRs are done too.

@liamhuber
Copy link
Member Author

But it's "draft" only in the sense that it should not be merged into main -- or at least main should not be released with this PR merged so safest not to merge it -- it is nonetheless ready for review.

@pmrv
Copy link
Contributor

pmrv commented Jun 3, 2024

I would consider ImportAlarm internal pyiron API, so I'd only be concerned about creating unnecessary conflicts with the other pyiron packages. I think by bumping minor here and removing them immediately is fine, if we bump all the involved packages together.

An alternative pattern I've used before is this, but I don't think it's necessary here.

Defining the objects/functions, but letting them raise exceptions at run time is the worst of both worlds imo, because it fails late.

@liamhuber
Copy link
Member Author

@pmrv this PR is also my least favourite route; the pattern you linked is definitely nicer than what I pushed here. My top choice would also be to simply remove them and bump the version sufficiently.

The only thing I'd push back on in your comment is that ImportAlarm is part of the internal API -- it's in the top-level __init__ under the comment # API of the pyiron_base module - in alphabetical order; while that doesn't specify public API, I think it's as close to such an assertion as we have anywhere. Regardless, with a significant enough version bump I'm comfortable simply changing the API.

@jan-janssen
Copy link
Member

My top choice would also be to simply remove them and bump the version sufficiently.

That would also be my preferred choice.

@liamhuber
Copy link
Member Author

Closed in favour of #1466, which just cleanly breaks compatibility by removing these from the API.

@liamhuber liamhuber closed this Jun 3, 2024
@liamhuber liamhuber deleted the change_api branch June 3, 2024 17:45
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.

3 participants