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

Grouping all comments in a chain at the beginning of it #708

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/comments/handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const {
} = require('../prettier-comments/language-js/comments');

const handleContractDefinitionComments = require('./handlers/ContractDefinition');
const handleMemberAccessComments = require('./handlers/MemberAccess');

function solidityHandleOwnLineComment(
comment,
Expand All @@ -26,6 +27,7 @@ function solidityHandleOwnLineComment(

if (
handleContractDefinitionComments(...handlerArguments) ||
handleMemberAccessComments(...handlerArguments) ||
handleOwnLineComment(comment, text, options, ast, isLastComment)
) {
return true;
Expand All @@ -52,6 +54,7 @@ function solidityHandleEndOfLineComment(

if (
handleContractDefinitionComments(...handlerArguments) ||
handleMemberAccessComments(...handlerArguments) ||
handleEndOfLineComment(comment, text, options, ast, isLastComment)
) {
return true;
Expand All @@ -78,6 +81,7 @@ function solidityHandleRemainingComment(

if (
handleContractDefinitionComments(...handlerArguments) ||
handleMemberAccessComments(...handlerArguments) ||
handleRemainingComment(comment, text, options, ast, isLastComment)
) {
return true;
Expand Down
19 changes: 19 additions & 0 deletions src/comments/handlers/MemberAccess.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
const {
util: { addDanglingComment }
} = require('prettier');

function handleMemberAccessComments(
text,
precedingNode,
enclosingNode,
followingNode,
comment
Comment on lines +6 to +10
Copy link
Member

Choose a reason for hiding this comment

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

not all these params have been used; maybe pass an object into the function instead of all these params?

) {
if (enclosingNode && enclosingNode.type === 'MemberAccess') {
addDanglingComment(enclosingNode, comment);
return true;
}
return false;
}

module.exports = handleMemberAccessComments;
Copy link
Member

Choose a reason for hiding this comment

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

Can this file be called also handleMemberAccessComments.js perhaps?

58 changes: 47 additions & 11 deletions src/nodes/MemberAccess.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
const {
doc: {
builders: { group, indent, label, softline }
builders: { group, hardline, indent, label, softline }
}
} = require('prettier');
const printComments = require('./print-comments');

const isEndOfChain = (node, path) => {
let i = 0;
Expand Down Expand Up @@ -94,33 +95,68 @@ const isEndOfChain = (node, path) => {
* be printed.
*/
const processChain = (chain) => {
const firstSeparatorIndex = chain.findIndex(
// Extract comments and reverse the order of print.
const comments = chain
.filter((element) => element.label && element.label === 'comments')
.reverse();
const chainContent = chain.filter(
(element) => !element.label || element.label !== 'comments'
);

const firstSeparatorIndex = chainContent.findIndex(
(element) => element.label === 'separator'
);
// The doc[] before the first separator
const firstExpression = chain.slice(0, firstSeparatorIndex);
const firstExpression = chainContent.slice(0, firstSeparatorIndex);
// The doc[] containing the rest of the chain
const restOfChain = group(indent(chain.slice(firstSeparatorIndex)));
const restOfChain = group(indent(chainContent.slice(firstSeparatorIndex)));

// We wrap the expression in a label in case there is an IndexAccess or
// a FunctionCall following this MemberAccess.
return label('MemberAccessChain', group([firstExpression, restOfChain]));
return label('MemberAccessChain', [
comments,
group([firstExpression, restOfChain])
]);
};

const MemberAccess = {
print: ({ node, path, print }) => {
print: ({ node, path, print, options }) => {
let comments;
const extractComments = (expressionPath) => {
const expression = expressionPath.getValue();
if (expression.type === 'FunctionCall') {
expressionPath.call(extractComments, 'expression');
return;
}
if (expression.type === 'IndexAccess') {
expressionPath.call(extractComments, 'base');
return;
}
comments = printComments(expression, path, options);
if (comments) {
comments = label('comments', [comments, hardline]);
}
};

path.call(extractComments, 'expression');

let expressionDoc = path.call(print, 'expression');
if (Array.isArray(expressionDoc)) {
expressionDoc = expressionDoc.flat();
}
expressionDoc = Array.isArray(expressionDoc)
? expressionDoc.flat()
: [expressionDoc];

const doc = [
expressionDoc,
comments,
...expressionDoc,
label('separator', [softline, '.']),
node.memberName
].flat();

return isEndOfChain(node, path) ? processChain(doc) : doc;
if (isEndOfChain(node, path)) {
extractComments(path);
return processChain([comments, doc].flat());
}
return doc;
}
};

Expand Down
4 changes: 2 additions & 2 deletions tests/config/format-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const RANGE_END_PLACEHOLDER = "<<<PRETTIER_RANGE_END>>>";

// Here we add files that will not be the same when formatting a second time.
const unstableTests = new Map(
["Comments/Comments.sol"].map((fixture) => {
["Comments/Comments.sol", "MemberAccess/MemberAccess.sol"].map((fixture) => {
const [file, isUnstable = () => true] = Array.isArray(fixture)
? fixture
: [fixture];
Expand All @@ -32,7 +32,7 @@ const unstableTests = new Map(

// Here we add files that will not have the same AST after being formatted.
const unstableAstTests = new Map(
[].map((fixture) => {
["MemberAccess/MemberAccess.sol"].map((fixture) => {
const [file, isAstUnstable = () => true] = Array.isArray(fixture)
? fixture
: [fixture];
Expand Down
53 changes: 53 additions & 0 deletions tests/format/MemberAccess/MemberAccess.sol
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,59 @@ contract MemberAccess {
path,
address(this, aoeu, aoeueu, aoeu)
);

// Comment before chain
game.
// CONFIG
config.
// Comment 1
// Comment 2
resolveWindow.
// Comment 3
functionCall(/* inside function comment */).
// Comment 4
array[/* inside array comment */ 1].
// Comment 5
call{value: 10, gas: 800}();


// Comment before chain
game.
// CONFIG
config.
// Comment 1
// Comment 2
resolveWindow.
// Comment 3
functionCall(/* inside function comment */).
// Comment 4
array[/* inside array comment */ 1].
// Comment 5
endOfChain;


// Comment before chain
game.
// CONFIG
config.
// Comment 1
// Comment 2
resolveWindow.
// Comment 3
functionCall(/* inside function comment */).
// Comment 4
endOfChain;


// Comment before chain
game.
// CONFIG
config.
// Comment 1
// Comment 2
resolveWindow.
// Comment 3
endOfChain;
}
}

Expand Down
106 changes: 106 additions & 0 deletions tests/format/MemberAccess/__snapshots__/jsfmt.spec.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,59 @@ contract MemberAccess {
path,
address(this, aoeu, aoeueu, aoeu)
);

// Comment before chain
game.
// CONFIG
config.
// Comment 1
// Comment 2
resolveWindow.
// Comment 3
functionCall(/* inside function comment */).
// Comment 4
array[/* inside array comment */ 1].
// Comment 5
call{value: 10, gas: 800}();


// Comment before chain
game.
// CONFIG
config.
// Comment 1
// Comment 2
resolveWindow.
// Comment 3
functionCall(/* inside function comment */).
// Comment 4
array[/* inside array comment */ 1].
// Comment 5
endOfChain;


// Comment before chain
game.
// CONFIG
config.
// Comment 1
// Comment 2
resolveWindow.
// Comment 3
functionCall(/* inside function comment */).
// Comment 4
endOfChain;


// Comment before chain
game.
// CONFIG
config.
// Comment 1
// Comment 2
resolveWindow.
// Comment 3
endOfChain;
}
}

Expand Down Expand Up @@ -195,6 +248,59 @@ contract MemberAccess {
path,
address(this, aoeu, aoeueu, aoeu)
);

// Comment before chain
// CONFIG
// Comment 1
// Comment 2
// Comment 3
// Comment 4
// Comment 5
Comment on lines +252 to +258
Copy link
Member

Choose a reason for hiding this comment

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

Is this what we would like to achieve? Move all the comments above the chain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes
Have a look at #644 and also try it on Prettier JS to see what they are doing at the moment.
It's not perfect but there is something nice in it.

Copy link
Member

Choose a reason for hiding this comment

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

I'll give it a try

game
.config
.resolveWindow
.functionCall() /* inside function comment */
.array[
/* inside array comment */
1
]
.call{value: 10, gas: 800}();

// Comment before chain
// CONFIG
// Comment 1
// Comment 2
// Comment 3
// Comment 4
// Comment 5
game
.config
.resolveWindow
.functionCall() /* inside function comment */
.array[
/* inside array comment */
1
]
.endOfChain;

// Comment before chain
// CONFIG
// Comment 1
// Comment 2
// Comment 3
// Comment 4
game
.config
.resolveWindow
.functionCall() /* inside function comment */
.endOfChain;

// Comment before chain
// CONFIG
// Comment 1
// Comment 2
// Comment 3
game.config.resolveWindow.endOfChain;
}
}

Expand Down