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

MarkdownBear should only use lint= if max_line_length is set #1581

Closed
coala-bot opened this issue Apr 4, 2017 · 6 comments
Closed

MarkdownBear should only use lint= if max_line_length is set #1581

coala-bot opened this issue Apr 4, 2017 · 6 comments

Comments

@coala-bot
Copy link

coala-bot commented Apr 4, 2017

Not using lint= will allow MarkdownBear to be used without the lint plugin, which means we may be able to support older versions of remark, but without the plugin functionality.

Unfortunately max_line_length is set to 80 by default, which was almost certainly a breaking change.
Either we:

  1. consider that a bad default and use None as the default, but then need to investigate what happens when two enabled bears have different defaults for the same config setting, and no user override.
  2. find a way for users to unset it. @NiklasMM had problems unsetting settings iirc, and ended up using \0
  3. allow max_line_length = 0 to mean no max length, basically unsetting it.
    difficulty/medium

Opened via gitter by @jayvdb

@NiklasMM
Copy link
Contributor

NiklasMM commented Apr 5, 2017

Yes, I had to set another value to None through the .coafile and found out that key = \0 does exactly that, which means it can be used to set non-None default values to None.

@satwikkansal
Copy link
Member

If I understood correctly, the issue is related to supporting both the old and new versions of remark-lint and remark-cli.

Is it a good idea to set the default value of max_line_length to 0 for MarkdownBear (which will mean unsetting it as long as the user doesn't provide a non-default value). And when value other than 0 is provided we run the bear with lint plugin (if possible)

@jayvdb
Copy link
Member

jayvdb commented Apr 18, 2017

@satwikkansal , this will assist with supporting both the old and new versions of remark-lint and remark-cli , because it would allow the user to de-activate remark-lint , which is where the compatibility problem comes from. But it also speeds up the bear as it doesnt need to load the remark-lint plugin.

Allowing max_line_length to be 0 (irrespective of whether it is user setting or the default bear setting) probably should involve changing all/many bears that use this setting, otherwise we create a usability/discoverability problem. I've created #1620 about that more general problem.

So the only remaining user-friendly solution for this bug is to change the default of max_line_length to None for this bear, which would be a second "breaking" change, but in fact it is that the bear wont emit an error when it previously did.

Note that MarkdownBear does not appear to be completely broken. I've isolated it to a limited scenario at #1235 (comment) . Still more analysis to be done however, as I havent been able to reproduce it on the coala-bears build infrastructure yet.

@jayvdb
Copy link
Member

jayvdb commented Apr 20, 2017

@satwikkansal , this will assist with supporting both the old and new versions of remark-lint and remark-cli , because it would allow the user to de-activate remark-lint , which is where the compatibility problem comes from. But it also speeds up the bear as it doesnt need to load the remark-lint plugin.

Allowing max_line_length to be 0 (irrespective of whether it is user setting or the default bear setting) probably should involve changing all/many bears that use this setting, otherwise we create a usability/discoverability problem. I've created #1620 about that more general problem.

So the only remaining user-friendly solution for this bug is to change the default of max_line_length to None for this bear, which would be a second "breaking" change, but in fact it is that the bear wont emit an error when it previously did. Here is an example of how the current default introduced a breaking change, so setting the default to None would be reverting that breaking change.

Note that MarkdownBear is not completely broken. It breaks if the NODE_PATH isnt set.

@jayvdb jayvdb self-assigned this Apr 20, 2017
jayvdb added a commit to jayvdb/coala-bears that referenced this issue Apr 20, 2017
The introduction of max_line_length with a default of 80
was a breaking change as previously line length was not
constrained.

This constraint required the use of remark-lint, which
caused MarkdownBear to remove all content from files
if remark-lint could not be found in NODE_PATH.

The default is now again to not check line length,
and not use remark-lint unless the .coafile explicitly
requests a max_line_length.

Fixes coala#1581
@satwikkansal
Copy link
Member

So the only remaining user-friendly solution for this bug is to change the default of max_line_length to None for this bear, which would be a second "breaking" change, but in fact it is that the bear wont emit an error when it previously did.

Yes, that seems like the only way to solve this issue. Wondering what happens when the remark-lint is not compatible and the user has provided non-default value for max_line_length, do we issue an informative warning to the user?

PS: Sorry, didn't reply to the comments because I accidentally turned off notifications for this issue.

@jayvdb jayvdb added this to the 0.10.2 milestone Apr 20, 2017
jayvdb added a commit to jayvdb/coala-bears that referenced this issue Apr 20, 2017
The introduction of max_line_length with a default of 80
was a breaking change as previously line length was not
constrained.

This constraint required the use of remark-lint, which
caused MarkdownBear to remove all content from files
if remark-lint could not be found in NODE_PATH.

The default is now again to not check line length,
and not use remark-lint unless the .coafile explicitly
requests a max_line_length.

Fixes coala#1581
@jayvdb
Copy link
Member

jayvdb commented Apr 20, 2017

when the remark-lint is not compatible

At the moment, we cant detect that, and do not even show that a problem occurred.
(We were just zero'ing the file)

The work going on in https://gitlab.com/coala/package_manager/ is going to give us tools to do proper analysis of the runtime environment, including non-python dependencies.

jayvdb added a commit to jayvdb/coala-bears that referenced this issue Apr 20, 2017
The introduction of max_line_length with a default of 80
was a breaking change as previously line length was not
constrained.

This constraint required the use of remark-lint, which
caused MarkdownBear to remove all content from files
if remark-lint could not be found in NODE_PATH.

The default is now again to not check line length,
and not use remark-lint unless the .coafile explicitly
requests a max_line_length.

Fixes coala#1581
gosom pushed a commit to gosom/coala-bears that referenced this issue Jul 15, 2017
The introduction of max_line_length with a default of 80
was a breaking change as previously line length was not
constrained.

This constraint required the use of remark-lint, which
caused MarkdownBear to remove all content from files
if remark-lint could not be found in NODE_PATH.

The default is now again to not check line length,
and not use remark-lint unless the .coafile explicitly
requests a max_line_length.

Fixes coala#1581
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants