Shift to solidity-parser from solparse about ethlint HOT 17 CLOSED

@duaraghav8 Somewhere federico suggested turning on Travis for this repo. Have you ever used that before? I think its simple to open an account that is linked to your GitHub and toggle Solium on. It's free for open source. Then I can submit a travis.yml that works with Solium for the wrapped test suite and you will be able to easily verify nothing has broken.

from ethlint.

This is my first time with travis.
I've synced Solium & pushed the .travis.yml

I'm guessing the next step is to push a commit?

from ethlint.

No no it's good. Thanks though!

from ethlint.

Hi. Hope you had a wonderful December :) I'd love to help out with this issue.

Do you have a list of critical differences between solparse and solidity-parser? Just talked with frederico about pulling in the good work you've done, forcing solidity-parser on you and then changing solidity-parser so that everything breaks! But then immediately fixing it. Any thoughts?

from ethlint.

@cgewecke happy new year :) That'd be great!

I haven't been active on Github lately (for which I feel terrible), so maintenance of Solium is over-due. I'll take a look at the current version of solidity-parser and run tests to dig out all the differences and will send you an email.
Please let me know your ID.

Thanks big time!

from ethlint.

I'm making a list here of things to do to stop solidity-parser from breaking Solium.

• unit tests will need to run a patched version of solidity-parser that modifies the SourceUnit rule to include Statement and SourceElement. This will allow the tests to pass statements, events and structs to the parser without any complaints about Pragma, Contract missing, etc.

from ethlint.

This PR at solidity-parser would impact Solium in the following places (that I can see). I don't know that it will be accepted but something similar is coming...

• rules/whitespace.js - Line 230. The new SP rule doesn't use AssignmentExpression when parsing the value assignment to StateVariableDeclaration. It could probably be written in a way that's easier to lint.

The following places would need to add handling for StateVariableDeclaration nodes.

• rules/mixedcase.js - Line 63:
• rules/no-unused-vars Line 23
• rules/uppercase Line 21
• variable-declarations Line 32

The tests for these cases would have to take the different contexts in which a StateVariableDeclaration and a DeclarativeExpression appear into account - e.g. the former at the contract level, the latter at the local.

Edit: Per #68. The contract might just be an interface. This rule doesn't need to be changed, using SP would resolve #68.

from ethlint.

Edited above to contain link to relevant PR.

from ethlint.

@duaraghav8 Hi. Have been thinking about how to integrate Solium's (majestic) test-suite with SP's new strictness about statement context and have been unable to come up with an elegant solution. It looks like the simplest and lowest-maintenance thing to do might be to bite the bullet now by:

• writing some simple wrapper functions that take any code snippet and locate it within a contract or function. Maybe put these in util/js-utils.js?
• going through the tests and wrapping all the snippets.
• making sure everything passes with solparse.

This would put Solium within striking distance of moving to SP. I am excited to work on a PR like this but I'd really like your input before doing so. It's a lot of cutting and pasting (which I enjoy, but still.)

If this seems reasonable and you have some architecture advice that would be wonderful. There may be a much better approach than this proposal - unfortunately I can't think of it.

from ethlint.

@cgewecke I think having a wrapper function is a great idea. That way, any one who reads/writes tests will only be focusing on the line(s) of code to be tested without the hassle of surrounding them all with contract declaration code.

You'll have to go through the tests once because some of them include the line to be tested wrapped inside the contract, so these tests need not use the wrapper function.

Looking forward to your PR!

Also, let me know if you'd be interested in becoming a collaborator for this project :)

from ethlint.

@duaraghav8 Ok perfect! I will work on that this week. I'd be very happy to be a collaborator, thank you. It would make it very straightforward for you to modify any PR's as you wish.

from ethlint.

@duaraghav8 Yes it worked! On a side-note - I think you should restrict my ability to push / merge to master in order to minimize the amount of damage I could do here. It must be possible in collaborator settings somewhere. Have opened a PR from the wrap-test-code branch. Feel free to edit or suggest revisions at your leisure.

Are PRs for fixes to SP anywhere on the horizon for you? No pressure - I'm just curious about whether the wrappers are ok.

from ethlint.

Also @duaraghav8 if it's simpler for you to delegate the SP fixes to me I'm happy to do them. Likewise if your intention is that I merge changes to Solium please let me know. Solium has such a robust test suite and is so well structured that new code can be added to it with a high degree of confidence.

And any guidance you can give me about what I should be doing would be welcome - I don't really know what the norms for collaboration are. Sorry if I've made this more complicated than it needs to be, lol. 😄

from ethlint.

Thanks @cgewecke, that's a huge compliment :D
I'll check out the wrap-test-code PR. I'd love it if you could push the SP fixes (sorry for being a lazy ass!)
In terms of coding style, just some basic things:
we're using semicolons, tabs and === for equality comparisons (if you have a good reason to use == instead, please accompany it with a comment).
I'm going to give priorities to the issues so it becomes very clear what will be worked on first.

I'll see if there's a way to restrict you from master branch :p
Let me know if I haven't addressed any of your concerns above

ps- not at all complicated, I love collaborating (I get to make new friends!). Its just that I haven't been able to find time for open source lately.

from ethlint.

Ok great! That's really helpful. Please don't be shy about suggesting revisions - stylistic or otherwise - for submissions. I'm always glad to improve something.

I'll get started on the SP stuff, thanks Raghav.

from ethlint.

@cgewecke thanks for all the PRs you've been working on for SP.
Regarding the note at the bottom, please mention yourself as the co-author of the fixes since I know you made changes to ensure things don't break in any of the dependants of the parsers.

from ethlint.

v0.5.0 onwards

from ethlint.