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.

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

Allow underscore in variable naming convention for internal state variables #2110

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

dokzai
Copy link
Contributor

@dokzai dokzai commented Sep 4, 2023

Hello!

This is my attempt at addressing #1034 based on the recommendation provided in the thread.

Thanks!

…e variables and update test_detector tests snapshot
@CLAassistant
Copy link

CLAassistant commented Sep 4, 2023

CLA assistant check
All committers have signed the CLA.

@@ -53,6 +53,7 @@ contract Test {

contract T {
uint private _myPrivateVar;
uint internal _myInternalVar;
uint _myPublicVar;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uint _myPublicVar;
uint public _myPublicVar;

Copy link
Member

Choose a reason for hiding this comment

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

Would you make all of the _myPublicVar's public and run python tests/e2e/detectors/test_detectors.py --compile to update the compiled artifacts please? You'll also want to run pytest tests/e2e/detectors/test_detectors.py --insta update to update the snapshots as the public variable should be included in the snapshot after updating the solidity code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

I updated the variable to public and updated the snapshots accordingly.

@@ -53,6 +53,7 @@ contract Test {

contract T {
uint private _myPrivateVar;
uint internal _myInternalVar;
uint _myPublicVar;
Copy link
Member

Choose a reason for hiding this comment

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

Would you make all of the _myPublicVar's public and run python tests/e2e/detectors/test_detectors.py --compile to update the compiled artifacts please? You'll also want to run pytest tests/e2e/detectors/test_detectors.py --insta update to update the snapshots as the public variable should be included in the snapshot after updating the solidity code.

@0xalpharush 0xalpharush merged commit ff52901 into crytic:dev Sep 8, 2023
74 checks passed
@devtooligan
Copy link
Contributor

@dokzai 🫡

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.

None yet

4 participants