-
Notifications
You must be signed in to change notification settings - Fork 85
rename global vars in tests #792
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
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
oflatt
left a comment
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.
Great work! We discussed and would actually like the script you used in the PR as well (:
Now that you have fixed this, could you also enable strict mode in all egglog tests in files.rs?
|
|
||
| (rule ((Wrap 1)) | ||
| ((let a (Wrap 1)))) No newline at end of file | ||
| ((let $a (Wrap 1)))) No newline at end of file |
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.
Probably we should also have a variant of this test where this line has (let a (Wrap 1)) which should also fail.
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.
While we are at it, if the rule comes before the global it should be allowed
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.
When I enable strict mode, all of the term_encoding tests fail. One way to fix this (I think) is to change the INTERNAL_PREFIX variable from @ to $@, that seems dirty because I have no idea if they're always supposed to be global.
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.
Oh I see. I'd say let's just disable strict mode for term encoding tests for now.
In the future I'll make the term encoding work by adding the global prefix when it generates names.
oflatt
left a comment
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.
Looks good!
| @@ -0,0 +1,40 @@ | |||
| #!/usr/bin/env python3 | |||
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.
Maybe this folder should be in a dev_utilities folder for clarity?
Working on #769. I wrote a script to parse a file and add the
$prefix to global variables that don't have it, another to use this to format all files, and third to verify that all files don't have compiler warnings.I left the scripts off the commit to avoid cluttering the repo with one-off scripts, but if they might be used in the future to format computer generated files, I can add them.