-
Notifications
You must be signed in to change notification settings - Fork 381
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
Allow config scopes with type annotations. #868
Conversation
Hi @vnmabus! I seem to have overlooked your PR. I like the idea to use the |
Hum, it worked in my PC. I can't see the errors in Azure (maybe because the runs have been removed). Can you relaunch the tests? |
When I try to run the testcases, I get the error |
GitHub doesn't let me re-run the checks. I clicked on the re-run button and it just disappeared without re-running the checks. The easiest is probably to push an empty commit, which should trigger the checks again. |
Conflicts: .gitignore sacred/config/config_scope.py
28f5135
to
96a4e20
Compare
It seems that it is working only in Python 3.8 and newer because of this: https://docs.python.org/3/whatsnew/3.8.html#tokenize . |
@vnmabus Agreed, if it makes your code significantly simpler, we can remove support for Python 3.7 |
I don't think it would be too hard to fix it either. But if you are planning to upgrade the minimum requirements soon I won't need to do it. |
I don't plan on upgrading the minimum requirements explicitly in a separate PR. The recent updates always happened as a byproduct of other changes, like this one. |
Well, probably it is cleaner to modify the files related with the minimum version (setup.py/pyproject.toml, tests, etc) in a separate PR. What do you think? |
Well, you are right. Are you going to open one? |
f1ee12d
to
96a4e20
Compare
I opened #887. |
I found one issue: def fn(): a = 2 # valid python code
get_function_body(fn) gives
The old variant threw an exception here. This now results in: from sacred.config.config_scope import ConfigScope
@ConfigScope
def fn(): a = 2
fn()
It should either work as expected or fail with a good error message |
Good catch!! I decided to make it work. I had to change the expected results of two tests, as now the comment after the function signature is also preserved (and the newlines too). |
I didn't find other issues, so I think it's ready to merge! |
Allows the usage of type hints in config scopes. Closes #818.
Notes:
ast
andtokenize
modules. Withast
we find the first line with code in the function body. As comments are removed in the AST representation, we then generate the tokens of the function until that line is reached, in order to keep the previous lines that only have comments and whitespace.