-
Notifications
You must be signed in to change notification settings - Fork 0
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
Mass testing fixes #199
Mass testing fixes #199
Conversation
This is the list of files (you can find the exact file on GitHub/PyPI) that I've found, will report back with a better list once I have a few common fixes ready: https://gist.github.com/isidentical/1762ae58c6ff41b29fc2ae970df63108 |
Seems like we are close to full roundtripping. Two cases that hit my eye (I also need to test my latest changes to ensure they don't break anything extra :D) that I don't have time today to fix (will take a look at them tomorrow if it is not fixed by then) # This seems like a bug with escaping
--- /tmp/ast-of/pypi/yt-dlp-2022.11.11/yt_dlp/extractor/common.py
+++ /tmp/ast-of/pypi/yt-dlp-2022.11.11/yt_dlp/extractor/common.py
@@ -2576,7 +2576,7 @@
Name(id='filename', ctx=Store())],
value=JoinedStr(
values=[
- Constant(value='\\\\?\\'),
+ Constant(value='\\?\\'),
FormattedValue(
value=Name(id='absfilepath', ctx=Load()),
conversion=-1)]))], # A lot of occurences, we should be able to eliminate these quite eazily since there is no real bug
--- /tmp/ast-of/pypi/docformatter-1.5.0/docformatter.py
+++ /tmp/ast-of/pypi/docformatter-1.5.0/docformatter.py
@@ -2622,6 +2622,7 @@
Return(
value=JoinedStr(
values=[
+ Constant(value=''),
FormattedValue(
value=Name(id='open_quote', ctx=Load()),
conversion=-1),
|
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.
These changes LGTM. Is there any reason why there is still a WIP in the title?
@@ -43,6 +43,10 @@ enum tokenizer_mode_kind_t { | |||
typedef struct _tokenizer_mode { | |||
enum tokenizer_mode_kind_t kind; | |||
|
|||
// TODO: we probably can infer this without storing it | |||
// from the other information available here. | |||
int format_spec; |
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.
We could also add a new tokenizer mode kind for this. I don't know what would simplify this more
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.
Was thinking about a mode, but since each mode currently maps to a single get function I thought it wasn't that necessary and this flag can kept in the mode level (and even removed in the future if we can leverage the other meta information here)
Thanks a lot for doing this check and fixing the errors @isidentical! We are so close! 🚀 |
Co-authored-by: Pablo Galindo Salgado <[email protected]>
I need to fix a final bug (the first example in here: #199 (comment)), but then we should be able to land this (which would mean we can roundtrip pretty much everything) |
Landing this now, will take care of the weird backslashes in the subsequent one. |
CC: @pablogsal @lysnikolaou
Kept each commit as a single fix, there are a couple more incoming.