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

Support no-visibility constructors in >=0.7.0 #241

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

junderw
Copy link
Contributor

@junderw junderw commented Jul 30, 2020

No description provided.

@junderw
Copy link
Contributor Author

junderw commented Jul 30, 2020

Though admittedly, since pre-0.7.0 visibility was required, but post-0.7.0 it was forbidden, anyone trying to glean rules about solidity by looking at the tests will be confused when they see a default visibility test in the with-visibility fixtures... without knowing it depends on the withContract function using >=0.7.0...

But I guess that's not what the tests are for... so it seems ok.

If you want to change it I will leave this branch open to edits by maintainers.

lib/config.js Outdated Show resolved Hide resolved
@junderw
Copy link
Contributor Author

junderw commented Aug 3, 2020

Looking forward to this. 👍

@junderw
Copy link
Contributor Author

junderw commented Aug 12, 2020

This should be good to merge. Any other problems that need fixing?

@fvictorio
Copy link
Contributor

fvictorio commented Aug 12, 2020

Hey @junderw, sorry for the delay (again).

I'm still not convinced about the pragma approach, it seems too brittle (again, a <0.7.0 would break it). I think the only alternative here is to add an option to the rule. I pushed a commit that does this. Can you check it out and tell me if that works for you?

@junderw
Copy link
Contributor Author

junderw commented Aug 12, 2020

That seems reasonable, but I think we should be actively alerting the user of this specific awkward instance.

  1. I updated the docs by fixing the meta info and generating the docs.
  2. I change the error message when it occurs on a constructor to alert people of the new option for >=0.7.0.

I think this is the best we can do in this situation... :-/

Co-authored-by: Franco Victorio <victorio.franco@gmail.com>
@junderw
Copy link
Contributor Author

junderw commented Aug 12, 2020

I have squashed the commits together.

The original commit structure at this point can be found here:

https://github.com/junderw/solhint/tree/oldPR

@junderw
Copy link
Contributor Author

junderw commented Aug 12, 2020

(Also I have rebased on protofire master)

@fvictorio
Copy link
Contributor

I updated the docs by fixing the meta info and generating the docs.

Thanks! I was going to do that and then I forgot 😅

I change the error message when it occurs on a constructor to alert people of the new option for >=0.7.0.

That's a great idea, thank you for doing it!

@fvictorio fvictorio merged commit 9ff1c56 into protofire:master Aug 12, 2020
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.

2 participants