Multiple watch args, future annotations, pytest asserts, and some other patches #35
Conversation
SamSokolin
changed the title
There was a problem hiding this comment.
This is great! Super excited to see things get updated! I'll try this out on https://github.com/heavy-resume/heavy-stack and see if it fixes some line number issues I was seeing updated the nested async functions called by reactpy
edit: I'll wait for @breuleux 's fix to decorators to see if they fix all the line number issues I encountered.
| def apply_assertion_rewrite(self, ast_func, glb): | ||
| from _pytest.assertion.rewrite import AssertionRewriter | ||
|
|
||
| # We only want to apply assertion rewrites if we're running in a pytest context | ||
| if not CONFIG.rewrite_asserts_to_pytest_asserts: | ||
| return ast_func |
There was a problem hiding this comment.
I think this will work as long as its off by default. One consideration is that a more robust solution is the ability to register hooks for reevaluate, then you would loop through those. I would for sure have that in a separate PR.
There was a problem hiding this comment.
The more I think about it, the more I think jurigged shouldn't own this, but rather just provide capabilities for other libraries to hook into it, which would be the hooks approach in this case. The pytest hot reloader library should own fixing pytest (which it already does), but it also should be able to do so without having to resort to monkey patching jurigged.
| # seem to have a way to add rewrite hooks | ||
| # TODO: We should fix this in jurigged itself, but porting over from here for now: | ||
| # https://github.com/breuleux/jurigged/issues/29 | ||
| new_node = self.apply_assertion_rewrite(new_node, glb) |
There was a problem hiding this comment.
consider moving the CONFIG check here so that the apply_assertion_rewrite logic is simpler. You wouldn't expect a function with that name to not do what it says.
There was a problem hiding this comment.
This comment is obsolete; after having time to stew on this, I don't think jurigged should own fixing pytest, it just needs to provide a means to do so without monkey patching the library.
| # patch: There's an off-by-one bug coming from somewhere in jurigged. | ||
| # This affects replaced functions. When line numbers are wrong | ||
| # the debugger and inspection logic doesn't work as expected. | ||
| # TODO: We should fix this in jurigged itself, but porting over from here for now: | ||
| # https://github.com/breuleux/jurigged/issues/29 |
There was a problem hiding this comment.
IIRC there's some places where the line number is tweaked. It would make sense to fix it there, but I would want @breuleux to comment
There was a problem hiding this comment.
As per #29 (comment) this was likely compensating for an issue with decorators and we can remove this since it wasn't a proper fix anyways
| def _get_future_compiler_flags(glb) -> int: | ||
| """use future feature names to get any that are set in the global namespace | ||
|
|
||
| `or` these together to get the flags for the compile function | ||
| """ | ||
| flags = 0 | ||
| for key in glb.keys() & _future_feature_names: | ||
| with suppress(Exception): | ||
| flags |= glb[key].compiler_flag | ||
| return flags | ||
|
|
||
|
|
||
| def _compile_with_flags(node, mode, filename, glb): | ||
| """compile the node with the future flags set in the global namespace | ||
|
|
||
| basically, respect `from __future__ import annotations` | ||
| """ | ||
| flags = _get_future_compiler_flags(glb) | ||
| return compile(node, mode=mode, filename=filename, flags=flags) |
There was a problem hiding this comment.
I would suggest removing this from this PR since its already in a different one.
|
Future annotations have been added from the other PR and I added the support for watch args in a different commit, done differently (e.g. -w x -w y instead of -w x y). For what remains, I'm not sure -- I'd rather not special case pytest rewrites and I haven't found a reliable fix for line numbers. |
This PR includes the following changes:
I'm happy to split these out into separate PRs or to refactor as necessary! Ideally we can find the root cause of the off by one error in the underlying codetools logic in jurigged rather than patching it retroactively.