Contract Definition comments #172

Janther · 2019-07-01T22:08:49Z

fixes #162

in this PR there are 2 new features.

the indentation on BlockComments is now aligned with the indentation of the subject of that comment.
many edge cases on comments around contract definitions are addressed.

input

contract A {
    // solhint-disable-previous-line no-empty-blocks
}

output won't even print currently.

after this PR

contract A {
    // solhint-disable-previous-line no-empty-blocks
}

Scenarios, where we were depending on prettier to guess where the comment should go, are now addressed.
input:

contract A /*comment about the name*/ is X /*interface X because ...*/, Y /*contract Y because ...*/, Z /*implements Z because ...*/ {
    // solhint-disable-previous-line no-empty-blocks
}

Output:

contract A is
    /*comment about the name*/
    X, /*interface X because ...*/
    Y, /*contract Y because ...*/
    Z /*implements Z because ...*/
    // solhint-disable-previous-line no-empty-blocks
{}

After this PR:

/*comment about the name*/
contract A is
    X, /*interface X because ...*/
    Y, /*contract Y because ...*/
    Z /*implements Z because ...*/
{
    // solhint-disable-previous-line no-empty-blocks
}

To address the Contract Definition edge cases I created the following functions
solidityHandleOwnLineComment, solidityHandleEndOfLineComment, and solidityHandleRemainingComment, these functions try to match the comments with Solidity's cases first and if no match was found, they call the javascript comment handler respectively.

My idea is to bring add Solidity's edge cases in this folder. and even redoing the javascript handlers in a way that makes sense with Solidity's AST.
With time we will rely less on the javascript handlers while fully using the utils.

Finally I had to add the package-lock.json because Travis kept installing the wrong eslint and failing to run the linting.

codecov · 2019-07-01T22:10:26Z

Codecov Report

Merging #172 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
+ Coverage   99.68%   99.70%   +0.02%     
==========================================
  Files          79       84       +5     
  Lines         628      677      +49     
  Branches      104      115      +11     
==========================================
+ Hits          626      675      +49     
  Misses          2        2

Impacted Files	Coverage Δ
src/comments/printer.js	`94.44% <0.00%> (ø)`
src/comments/handler.js	`100.00% <0.00%> (ø)`
src/comments/index.js	`100.00% <0.00%> (ø)`
src/comments/handlers/ContractDefinition.js	`100.00% <0.00%> (ø)`
src/nodes/print-comments.js	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5bb07f...fb9d154. Read the comment docs.

mattiaerre · 2019-07-02T04:13:42Z

.gitignore

@@ -62,6 +62,4 @@ typings/

 .DS_Store

-package-lock.json


not sure if I like this idea, we may want to instruct travis to use the proper eslint version perhaps

There's no rush for this PR so I'll see if after a rebase there is some improvement.

I rebased the branch to the latest release with the updated dependencies and the travis uses the right eslint without needing package-lock.json

@mattiaerre I'm curious, why don't you like this idea? AFAIK versioning the package-lock.json or yarn.lock is a pretty standard practice.

mattiaerre · 2019-07-02T04:14:03Z

src/clean.js

@@ -1,6 +1,6 @@
 // eslint-disable-next-line no-unused-vars
 function clean(ast, newObj, parent) {
-  ['code', 'codeStart', 'loc', 'range'].forEach(name => {
+  ['code', 'codeStart', 'loc', 'range', 'raw'].forEach(name => {


even here, not sure if this is how we should be dealing w/ this

in my opinion, comments shouldn't even be in AST comparison.
Prettier throws an exception already if we failed to print a comment.

mattiaerre · 2019-07-02T04:14:36Z

src/index.js

-const { handleComments } = require('./prettier-comments');
+// const { handleComments } = require('./prettier-comments');
+// const printComment = require('./comments/printer');
+const { handleComments, printComment } = require('./comments');


uhm interesting

mattiaerre · 2019-07-02T04:15:04Z

tests/Proxy/__snapshots__/jsfmt.spec.js.snap

-    * @dev Tells the address of the implementation where every call will be delegated.
-    * @return address of the implementation to which it will be delegated
-    */
+     * @dev Tells the address of the implementation where every call will be delegated.


I like this 👍

me too. maybe I could split this PR into

fixing comment indentation.

Contract Definition edge cases

fvictorio · 2019-08-02T18:13:44Z

@Janther What is this PR missing? Can I help somehow?

Janther · 2019-08-04T02:16:01Z

@fvictorio sorry for the wait, actually I'm happy with the PR but I'm open to improvements/suggestions to be made to it.

As stated in #181 the parser will begin including natspec comments from version 0.5.0 and that breaks some things so I believe we should start processing comments in a solidity way instead of a JS way, and I believe this is the first step towards that so I'd like us all to be on board with it.

mattiaerre · 2019-09-23T05:29:48Z

can you resolve the conflicts here too @Janther ? thanks

yxliang01 · 2019-09-24T08:41:28Z

Looking forward to this PR being merged! This is really important to my project :) & thanks for your work!

mattiaerre · 2019-09-29T03:14:02Z

I'd be inclined to merge this PR; shall we add more tests around it so that the code coverage will not drop too much?

…Definition.

…fferent leading whitespace.

.

Janther · 2020-02-24T23:46:01Z

Conflicts resolved, not sure why codecov is still angry, basically the only 2 lines that are not covered are the exception when there's no printer for this node (the parser had an update) or there's no printer for this comment (extract comments had an update).

please have a look @mattiaerre @fvictorio

mattiaerre · 2020-02-25T04:12:25Z

the problem here is that we've removed raw from the clean function and so codecov is complaining as it runs tests w/o the AST_COMPRARE flag; I'll be happy to go ahead and merge as it is since this is only affecting comments; what do you think @Janther @fvictorio ?

actually I am not entirely sure about this ^^ 🤔 let me spend some more time on this

mattiaerre · 2020-02-25T04:26:32Z

FYI:

https://community.codecov.io/t/mismatch-between-project-and-patch-coverage/1059

// cc @Janther @fvictorio

fvictorio · 2020-02-25T15:09:00Z

I'm OK with merging this.

jerrode · 2020-02-27T17:33:57Z

Hey everyone, Jerrod from @codecov here. Love the high coverage / testing approach you've taken.

Quick note: The check that is failing is patch coverage not project coverage. Patch = the coverage on the incremental branch, which basically means you must hit 100% coverage on added code.

If there is a specific test that isn't covering that you think should be, have you checked the underlying coverage report to see if the line is covered out of your CI?

https://codecov.io/gh/prettier-solidity/prettier-plugin-solidity/commit/a5bb07fcb86ba1c579f44090bb7558d66fbd22a0/build

fvictorio · 2020-02-27T18:29:33Z

Hi @jerrode, thanks for the help!

@Janther @mattiaerre Apparently the uncovered bit is in printer.js: the throw in the default case for the comment type. It sounds like we are just being defensive there, so I think we can just ignore it.

mattiaerre · 2020-02-27T19:17:25Z

thanks so much for pointing that out @jerrode that's awesome. so you are suggesting @fvictorio that we can just merge this as is? I'm fine w/ that

fvictorio · 2020-02-27T21:06:46Z

It's up to @Janther, but yes, I'm ok with merging this.

Janther · 2020-02-27T21:09:22Z

I’m happy to merge, I know what’s not being covered, I just don’t know why codecov is that angry hehe

Janther · 2020-02-27T21:17:15Z

Can’t merge from my phone though

mattiaerre reviewed Jul 2, 2019

View reviewed changes

Janther force-pushed the bugfix/contract_definition_comments branch 2 times, most recently from 549bdaa to f910da2 Compare July 8, 2019 22:12

Janther mentioned this pull request Aug 4, 2019

solidity-parser-antlr-v0.5.0 #181

Closed

Janther force-pushed the bugfix/contract_definition_comments branch from f910da2 to 707659e Compare September 25, 2019 01:34

Janther added 9 commits February 25, 2020 12:17

Extracted the logic of Block comments and also applied it to Contract…

53ec37b

…Definition.

upgrading to the latest print comments from language-js

cdbf871

Now we are properly indenting BlockComments so the raw will have a di…

1be2222

…fferent leading whitespace.

Extra comments tests

66915cc

refactor + ContractStatement comments

5b99726

adding comments explaining each scenario

ec90db0

.

using the original prettier-comments

a25af0e

refactor

188ced8

fixing rebase

fb9d154

Janther force-pushed the bugfix/contract_definition_comments branch from 707659e to fb9d154 Compare February 24, 2020 23:22

Janther merged commit 794f8a6 into master Feb 27, 2020

Janther deleted the bugfix/contract_definition_comments branch February 27, 2020 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contract Definition comments #172

Contract Definition comments #172

Janther commented Jul 1, 2019

codecov bot commented Jul 1, 2019 •

edited

Loading

mattiaerre Jul 2, 2019

Janther Jul 3, 2019

Janther Jul 8, 2019

fvictorio Aug 2, 2019

mattiaerre Jul 2, 2019

Janther Jul 3, 2019

mattiaerre Jul 2, 2019

mattiaerre Jul 2, 2019

Janther Jul 3, 2019

fvictorio commented Aug 2, 2019

Janther commented Aug 4, 2019

mattiaerre commented Sep 23, 2019

yxliang01 commented Sep 24, 2019

mattiaerre commented Sep 29, 2019 •

edited

Loading

Janther commented Feb 24, 2020

mattiaerre commented Feb 25, 2020 •

edited

Loading

mattiaerre commented Feb 25, 2020

fvictorio commented Feb 25, 2020

jerrode commented Feb 27, 2020

fvictorio commented Feb 27, 2020

mattiaerre commented Feb 27, 2020

fvictorio commented Feb 27, 2020

Janther commented Feb 27, 2020

Janther commented Feb 27, 2020

Contract Definition comments #172

Contract Definition comments #172

Conversation

Janther commented Jul 1, 2019

codecov bot commented Jul 1, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fvictorio commented Aug 2, 2019

Janther commented Aug 4, 2019

mattiaerre commented Sep 23, 2019

yxliang01 commented Sep 24, 2019

mattiaerre commented Sep 29, 2019 • edited Loading

Janther commented Feb 24, 2020

mattiaerre commented Feb 25, 2020 • edited Loading

mattiaerre commented Feb 25, 2020

fvictorio commented Feb 25, 2020

jerrode commented Feb 27, 2020

fvictorio commented Feb 27, 2020

mattiaerre commented Feb 27, 2020

fvictorio commented Feb 27, 2020

Janther commented Feb 27, 2020

Janther commented Feb 27, 2020

codecov bot commented Jul 1, 2019 •

edited

Loading

mattiaerre commented Sep 29, 2019 •

edited

Loading

mattiaerre commented Feb 25, 2020 •

edited

Loading