Whitespace stripped from markdown despite settings, doesn't work with `text.md` scope

thoschworks commented 5 years ago

Edit by @rsese to describe the larger issue

As mentioned in https://github.com/atom/whitespace/issues/185#issuecomment-446533137:

It's not as much that language-markdown doesn't respect the setting, but whitespace should now also work within the text.md scope.

I believe https://github.com/atom/whitespace/pull/177 was supposed to add this functionality so that the whitespace package worked with the text.md scope but as mentioned in https://github.com/atom/whitespace/issues/185#issuecomment-448727955 (the author of #177), it looks like it may not have done what was intended.

The result is that if you use language-markdown (as in this report), the whitespace package will still strip trailing whitespace on save in Markdown files even if you have Keep Markdown Line Break Whitespace enabled.

Prerequisites

[X] Put an X between the brackets on this line if you have done all of the following:
- Reproduced the problem in Safe Mode: http://flight-manual.atom.io/hacking-atom/sections/debugging/#using-safe-mode
- Followed all applicable steps in the debugging guide: http://flight-manual.atom.io/hacking-atom/sections/debugging/
- Checked the FAQs on the message board for common solutions: https://discuss.atom.io/c/faq
- Checked that your issue isn't already filed: https://github.com/issues?utf8=✓&q=is%3Aissue+user%3Aatom
- Checked that there is not already an Atom package that provides the described functionality: https://atom.io/packages

Description

Whitespace is stripping trailing whitespaces in markdown files despited the settings.

Edit by @rsese: not reproducible in safe mode because it reproduces when you install and use https://atom.io/packages/language-markdown

Steps to Reproduce

Edit a README.md for a project
Put two whitespaces at end of line to indicate a line break.
Save the file.

Expected behavior: The whitespace at the end of the line are ignored and and not deleted.

Actual behavior: The whitespaces are deleted. (The trailing whitespaces in the current line are ignored.) If the option "Remove Trailing Whitespace" or the package Whitespace is deactivated, the whitespaces remain.

Reproduces how often: 100.0000000%

Versions

Atom : 1.33.0 Electron: 2.0.11 Chrome : 61.0.3163.100 Node : 8.9.3

Whitespace: 0.37.7

OS: Mac OS Mohave 10.14.1

Additional Information

I can not reproduce the issue running Atom on Debian (Bunsen Labs).

rsese commented 5 years ago

Thanks for the report - I'm unable to reproduce with 1.33.0 on macOS 10.12.6. Can you share your settings for Ignore Whitespace On Current Line and Keep Markdown Line Break Whitespace as well?

thoschworks commented 5 years ago

Can you share your settings for Ignore Whitespace On Current Line and Keep Markdown Line Break Whitespace as well?

Both are activated.

rsese commented 5 years ago

Hmm, I wonder if there's some setting/config somewhere causing the issue? If you temporarily reset to factory defaults, can you still reproduce?

thoschworks commented 5 years ago

I have no access to the machine for the next days. Stay tuned…

thoschworks commented 5 years ago

After resetting to the factory settings, the problem was solved until I reinstalled the package language-markdown. (On the linux machine is not installed…)

I will report a bug for language-markdown.

thoschworks commented 5 years ago

After resetting to the factory settings, the problem was solved until I reinstalled the package language-markdown. (On the linux machine is not installed…)

I will report a bug for language-markdown.

Or perhaps it's the combination of language-markdown and whitespace: language-markdown changes an attribute and whitespace can not recognize the source as markdown?

rsese commented 5 years ago

the problem was solved until I reinstalled the package language-markdown. (

There's an existing issue for this on that package repository:

https://github.com/burodepeper/language-markdown/issues/200

Looks like it was closed as resolved in https://github.com/atom/whitespace/pull/177 and that change is available in 1.33.0.

@burodepeper :wave: - am I misunderstanding https://github.com/atom/whitespace/pull/177? It seems like language-markdown should now respect whitespace's Keep Markdown Line Break Whitespace setting after your change?

burodepeper commented 5 years ago

@rsese It's not as much that language-markdown doesn't respect the setting, but whitespace should now also work within the text.md scope. I can't remember to what extend I've tested it. Maybe this can be added to this package's spec?

thoschworks commented 5 years ago

@rsese It's not as much that language-markdown doesn't respect the setting, but whitespace should now also work within the text.md scope. I can't remember to what extend I've tested it. Maybe this can be added to this package's spec?

This supports my assumtion, that there is a problem with combination of whitespace and language-markdown.

After adding

".text.md":
    whitespace:
    removeTrailingWhitespace: false

to my config.cson, the problem is solved.

I agree with @burodepeper that whitespace should also respect the scope text.md as markdown file.

burodepeper commented 5 years ago

I found an issue in my PR in #177 and I've submitted a new PR #186 to fix it. I wrote silly Javascript, I take the blame.

rsese commented 5 years ago

Just circling back to this, thanks @burodepeper! I'll edit this issue body today so it's clear the problem is that the whitespace package isn't working with the text.md scope as it was meant to after #177.

ivanpu commented 5 years ago

Um... I made a PR for this, but it was attached to the wrong repository... is it possible to migrate it, or should I create a new one from scratch?

burodepeper commented 5 years ago

I haven't had time to add tests to my previous PR. If you find time to do it for yours, feel free to do so, then I'll remove my PR.

ivanpu commented 5 years ago

I don't know how to add tests, and couldn't find how to test manually on my machine (couldn't find this package files within atom directories). Is there some documentation for this?

rsese commented 5 years ago

Our documentation on writing tests is here (it covers running tests as well):

https://flight-manual.atom.io/hacking-atom/sections/writing-specs/

If you need help writing automated tests for Atom, check out Discuss, the official Atom and Electron message board or join the Atom Slack team. There are a bunch of helpful community members that should be willing to point you in the right direction (I'm not an engineer so can't help you myself unfortunately).

thoschworks commented 5 years ago

@rsese

or join the Atom Slack team.

This link is down for months...

rsese commented 5 years ago

I messaged you on the message board @thosch66, we can talk more there.

thoschworks commented 5 years ago

@rsese Thanks. I found the thread in the message board after posted here.

ivanpu commented 5 years ago

@rsese Thanks, will look at it, hopefully soon - I'm too busy these days with work.

atom / whitespace