-
Notifications
You must be signed in to change notification settings - Fork 991
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
Raise error when a generator is defined in generators and generate() #12722
Raise error when a generator is defined in generators and generate() #12722
Conversation
…d in generate() This used to be a mistake, but no error was ever given to the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beware, it happens that the VirtualBuildEnv
and VirtualRunEnv
are implicit, even if they are not declared, they are called after generate()
.
I think that is the thing breaking the tests.
Yeah, I had some errors locally with integration tests that looked like remote issues, and this slipped thru the cracks when checking for real errors. Most of the were something like this, let's discuss it tomorrow: Example of one of the test errors I'm getting, click to expand
|
conan/tools/apple/xcodedeps.py
Outdated
@@ -105,6 +105,10 @@ class XcodeDeps(object): | |||
""") | |||
|
|||
def __init__(self, conanfile): | |||
if self.__class__.__name__ in conanfile.generators: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's duplicated multiple times, maybe it makes sense to extract code to its own function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that this would take just 2 lines, the if
and then 1 line for the error message, not 3.
So @RubenRBS do whatever you think it is best, I am happy with either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only issue with this is the thing we mentioned on friday that it's not obvious where to place such helper function. I've done so in tool's init, but happy to change. Thanks @SSE4 for your suggestion :)
I've noticed that VirtualRunEnv and VirtualBuildEnv are instantiated in lots of places just to handle some envs, even though the generator is defined in the attribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importing a private function from another module can be a linting or style error, we might need to rename it (later, not now), and if there are concerns of being accessible for users, it should be moved to internal
namespace.
yes, I think |
Co-authored-by: James <memsharded@gmail.com>
Co-authored-by: James <memsharded@gmail.com>
Co-authored-by: James <memsharded@gmail.com>
Thanks for your suggestion @SSE4. I ended up adding it to |
This used to be a mistake, but no error was ever given to the user
Changelog: (Feature): Raise an error when a generator is both defined in generators attribute and instantiated in generate() method
Docs: Omit
Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.