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

Plugin.__version__ and Plugin.version are ambigous #688

Open
JoranAngevaare opened this issue Apr 25, 2022 · 3 comments
Open

Plugin.__version__ and Plugin.version are ambigous #688

JoranAngevaare opened this issue Apr 25, 2022 · 3 comments
Labels
bug Something isn't working

Comments

@JoranAngevaare
Copy link
Member

JoranAngevaare commented Apr 25, 2022

Plugin.__version__ is used where Plugin.version should be used. Examples:

After writing #690, I'm starting to seriously doubt the usefulness of having a version that depends on the run_id (which isn't even generated from the Plugin.run_id attribute).

@JoranAngevaare JoranAngevaare added the bug Something isn't working label Apr 25, 2022
This was referenced Apr 25, 2022
@WenzDaniel
Copy link
Collaborator

Yes I think having run_id dependent versions does not make any sense anymore. To me

strax/strax/plugin.py

Lines 237 to 243 in a0d51fd

def version(self, run_id=None):
"""Return version number applicable to the run_id.
Most plugins just have a single version (in .__version__)
but some may be at different versions for different runs
(e.g. time-dependent corrections).
"""
return self.__version__
looks like some legacy code and with CMT2.0 certainly not required anymore.

@WenzDaniel
Copy link
Collaborator

But I am wondering if we should not try to be consistent in our code and either use always __version__ or .version(). I would be in favor of the former. If we do the later we might want to make it a property.

@JoranAngevaare
Copy link
Member Author

But I am wondering if we should not try to be consistent in our code and either use always
Right, this issue is precisely about this 😉

I would also be in favor of dropping the version class method (see my later addendum to this issue). I'm not super thrilled about this PR: #690. I can make #689 still work easily if we drop the .version-method by setting the __getattr__ method appropriately.

My main issue is that the version method can now contain all kinds of weird code, which requires you to init the plugin in order to know what the version is going to be, which is slow. Making it a classmethod would already help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants