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

review feat: Enable comments by default #2065

Merged
merged 4 commits into from
Jun 19, 2018

Conversation

surli
Copy link
Collaborator

@surli surli commented Jun 14, 2018

This PR is a proposal to change a default behaviour in Spoon: until now, the default was to not take into account the comments.
I assume this behaviour was used because the comments were not properly parsed then. But I think it's not the case anymore and we got more users using Spoon for parsing comments: very recently we got a question on Stackoverflow linked to that: https://stackoverflow.com/questions/50780598/how-to-get-comments-using-spoon/ and I know we also got issues related to that.

So in the same way we decided to use noclasspath mode by default, I propose that we parse comments by default. WDYT?

@tdurieux
Copy link
Collaborator

The comment was not enable for performance reasons.

@surli
Copy link
Collaborator Author

surli commented Jun 14, 2018

The comment was not enable for performance reasons.

Do we have an idea about the perf difference on a large code base?

@tdurieux
Copy link
Collaborator

We need to read again all the Java files.
It is no terribly slow but it has an impact.

@surli surli changed the title feat: Enable comments by default review feat: Enable comments by default Jun 15, 2018
@monperrus
Copy link
Collaborator

I'm rather OK for this new default behavior.

@surli surli changed the title review feat: Enable comments by default WiP feat: Enable comments by default Jun 15, 2018
@surli surli changed the title WiP feat: Enable comments by default review feat: Enable comments by default Jun 18, 2018
@monperrus monperrus merged commit fb9baae into INRIA:master Jun 19, 2018
@monperrus
Copy link
Collaborator

THanks. We have to prepare a really good changelog with all the non-bacward compatible changes such as this one.

if (jsapActualArgs.getBoolean("enable-comments")) {
Launcher.LOGGER.warn("The option --enable-comments (-c) is deprecated as it is now the default behaviour in Spoon.");
} else {
Launcher.LOGGER.warn("Spoon now parse by default the comments. Consider using the option --disable-comments if you want the old behaviour.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: parse -> parses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants