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

mark something as not a hook #249

Open
RonnyPfannschmidt opened this issue Jan 17, 2020 · 10 comments
Open

mark something as not a hook #249

RonnyPfannschmidt opened this issue Jan 17, 2020 · 10 comments

Comments

@RonnyPfannschmidt
Copy link
Member

follow-up to pytest-dev/pytest#6475

sometimes naming choices/backward compatibility leave functions in a name that may inadvertent fit a plug-in system

while its easy to mark things as optional hook to hide the error, that's also a time bomb waiting for someone to create that hook

a distinct marking for "really not a hook" should resolve that more clearly

@marcelotrevisani
Copy link

marcelotrevisani commented May 31, 2020

Hi @RonnyPfannschmidt ,
let me see if I understood this problem and I am thinking that I may tackle it
For the fix, we will need to add a mark to the function and all the time which we add a new hook to the LIFO list it will check if that mark is present, if it is present we will not add it to the list, am I right?
Just want to understand it right and if the solution is in the right path

@RonnyPfannschmidt
Copy link
Member Author

@marcelotrevisani this, and we need to ignore them if they match the naming scheme but have no hook declared

@marcelotrevisani
Copy link

@marcelotrevisani this, and we need to ignore them if they match the naming scheme but have no hook declared

Thanks!
Just one more thing, where can I find the whole naming scheme which I should check? Should I skip just the whole functions which have the same name as any of those HookimplMarker, HookspecMarker, etc ?

@RonnyPfannschmidt
Copy link
Member Author

an example of the porblem is demonstrated in the pytest issue - the basic problem is that when pluggy is instructed to search for hooks having a prefix, even if they are not marked explicitly as hookimpl, name clashes trigger issues -

@RonnyPfannschmidt
Copy link
Member Author

so basically a function named in a way that could be interpreted as unknown hook, we should be able to mark it as "certainly not a hook" as ignoring it or renaming it is still a potential time bomb

@marcelotrevisani
Copy link

so basically a function named in a way that could be interpreted as unknown hook, we should be able to mark it as "certainly not a hook" as ignoring it or renaming it is still a potential time bomb

got it! Thanks for the explanation! :)

@wahuneke
Copy link
Contributor

It looks like progress here may have stalled, so I took a crack at it. @RonnyPfannschmidt please tell me your thoughts about the draft solution in PR #447 . Am I in the ballpark? Please let me know your feedback on naming conventions, coding conventions, gotchas, or of course anything else! I don't mind nitpicks, I'd like my contribution to fit in with everything else you already have.

@bluetech
Copy link
Member

I reckon this is a pytest-only problem, because pluggy requires @hookimpl decoration. I don't think it makes sense to add a solution in pluggy to a non-existent problem.

pytest adds pytest_* auto-discovery which causes the issue. So the solution should go in pytest.

while its easy to mark things as optional hook to hide the error, that's also a time bomb waiting for someone to create that hook

Can combine with specname='notahook-uG4xoj4HIlYnwoPGItmis', no risk there :)

@wahuneke
Copy link
Contributor

That would also get around the uncomfortable element in the code I'm submitting, where I catch all exceptions in order to get around some pytest weirdness. Fewer instances of 'catch all', the better for sure.

If this is really of no use to any project other than pytest and it's only a workaround for people who are ignoring the correct practice of using explicit hookimpl, I can certainly see the good side of not merging this solution and closing the ticket...

@RonnyPfannschmidt
Copy link
Member Author

There was a time where pluggy would do prefix matching, it was deprecated and moved to pytest

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

No branches or pull requests

4 participants