Skip to content

Bug fixes, bring back line numbers#9

Merged
mds1 merged 7 commits intomainfrom
fixes
Dec 2, 2022
Merged

Bug fixes, bring back line numbers#9
mds1 merged 7 commits intomainfrom
fixes

Conversation

@mds1
Copy link
Contributor

@mds1 mds1 commented Nov 16, 2022

Fixes a few things that broke in #8, and reintroduces line numbers for findings. See commit messages for details

Copy link
Contributor

@jferas jferas left a comment

Choose a reason for hiding this comment

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

Looks good.. nice to have the line numbers back.. and parsing is so elegant with solang.

Copy link
Member

@apbendi apbendi left a comment

Choose a reason for hiding this comment

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

This worked, inasmuch as it properly ignored a contract in my script/ dir that was not actually a script.

It also failed when I commented out run from a contract that is a script. However, the error message it produced gave no hint as to why:

gitcoin-governor % scopelintl check
error: Formatting validation failed, run `scopelint fmt` to f

versus the current output:

% scopelint check
Invalid script interface in ./script/Deploy.s.sol
error: Naming conventions failed, see details above
error: Formatting validation failed, run `scopelint fmt` to fix

Copy link

@davidlaprade davidlaprade left a comment

Choose a reason for hiding this comment

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

LGTM, and ran without issue on flex voting 👍

Copy link
Member

@apbendi apbendi left a comment

Choose a reason for hiding this comment

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

Looks good to me! Seems to work when run against the Gitcoin Governor codebase.

One minor nit is that the error message for script naming says " Scripts must have a single public method named run" but it also seems to allow a public setUp method. Perhaps worth updating to say that? Will leave up to you—otherwise good to merge.

@mds1 mds1 merged commit d3bff51 into main Dec 2, 2022
@mds1 mds1 mentioned this pull request Dec 8, 2022
9 tasks
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.

4 participants