Skip to content

Rewrite assert statements, refs #12#13

Open
sobolevn wants to merge 3 commits intoisidentical:mainfrom
sobolevn:assert-stmt
Open

Rewrite assert statements, refs #12#13
sobolevn wants to merge 3 commits intoisidentical:mainfrom
sobolevn:assert-stmt

Conversation

@sobolevn
Copy link
Copy Markdown

@sobolevn sobolevn commented Feb 28, 2022

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
Copy Markdown
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.

Copy link
Copy Markdown
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor assert x == y into self.assertEqual(x, y)

2 participants