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.

Sign up for GitHub

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

Jump to bottom

Rewrite assert statements, refs #12 #13

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Rewrite assert statements, refs #12 #13

wants to merge 3 commits into from

Conversation

sobolevn
Copy link

@sobolevn sobolevn commented Feb 28, 2022 •
edited
Loading

This is still a WIP.

I have several questions:

  1. How does get_arg_offset suppose to work? I am bit a lost with this approach 🙂
  2. Do we need to handle msg part of assert expr, msg? Because not all self.assert*() methods support it

Closes #12

@isidentical
Copy link
Owner

How does get_arg_offset suppose to work? I am bit a lost with this approach

Yeah, it is not a very straight forward solution. But basically it is for adjusting comments appropriately when the initial version of the same call (e.g assertTrue(a < 10, msg="x") has 2 arguments) has more/less arguments than the refactored version (e.g assertLessEquali(a, 10, msg="x") has 3 arguments).

I don't think it is something that this refactor should worry about. Perhaps try returning 0?

Do we need to handle msg part of assert expr, msg? Because not all self.assert*() methods support it

I guess it serves the same purpose, so we can use it. I don't think we need to add it in this PR.

isidentical
isidentical reviewed Mar 1, 2022
View reviewed changes
Copy link
Owner

@isidentical isidentical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look in detail yet, but this is impressive work (I thought the existing refactoring infrastructure wouldn't work with asserts, but from what I understand by looking at the tests it seem to work pretty well.)

One Q: There is a special mode in teyit called --show-stats, which dumps the refactor results after each run. I wonder whether we can integrate this to there, so we can see how many asserts get refactored. It is a pretty useful metric, and if you see the Teyit step in CPython run it helps us to ensure we don't make any problematic change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isidentical isidentical isidentical left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor assert x == y into self.assertEqual(x, y)
2 participants
@sobolevn @isidentical