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

Formatter off on #289

Closed
wants to merge 6 commits into from
Closed

Conversation

nonomoho
Copy link
Member

fix #287

@pascalgrimaud
Copy link
Member

good job @nonomoho !

Copy link
Member

@Shaolans Shaolans 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 ! Well done @nonomoho !

Could you remove single-printer-run/_input.java and single-printer-run/_output.java from the commited files ?

packages/java-parser/src/comments.js Outdated Show resolved Hide resolved
Copy link
Contributor

@clementdessoude clementdessoude left a comment

Choose a reason for hiding this comment

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

LGTM ! Thanks @nonomoho !

Just two additionals comments:

  • it would be great to find crystal clear names for the formatter-on-off functions to distinguish them from the prettier-ignore ones. Maybe separate them in the code and add jsdoc ?
  • in the same idea, we could try to call only one method in packages/java-parser/src/index.js which contains the logic regarding ignore-comments (prettier-ignore and formatter:(on|off)). Same in packages/java-parser/src/parser.js: it would be clearer IMO to just call a shouldIgnore function in the parser file, which would then call a shouldIgnoreOffOn/PrettierIgnore or something like that

packages/java-parser/src/comments.js Outdated Show resolved Hide resolved
packages/java-parser/src/comments.js Outdated Show resolved Hide resolved
packages/java-parser/src/comments.js Outdated Show resolved Hide resolved
@clementdessoude
Copy link
Contributor

Would it be possible to add some tests at some other places than just before classes definition ? Before methods, binary operations and so on

@clementdessoude
Copy link
Contributor

Closed with #323

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.

Handle @formatter:off and @formatter:on ?
4 participants