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

[editorconfig]: All md files except in test use tab #1111

Merged
merged 2 commits into from
Mar 14, 2018
Merged

[editorconfig]: All md files except in test use tab #1111

merged 2 commits into from
Mar 14, 2018

Conversation

joshbruce
Copy link
Member

@joshbruce joshbruce commented Mar 3, 2018

Marked version: 0.3.17

Description

tl;dr: PR changes editorconfig to use tabs not spaces while still ignoring MD files in test directory.

Looking at the CommonMark specification and considering the future regarding #1104 and #1103 (comment) a lot of the examples use honest tabs...not spaces in Markdown...makes sense given the nature of plain text editors (this was why @davisjam's initial contributions didn't pass lint - he uses VIM, which doesn't do spaced tabs).

So, not too sure what to do here regarding ignoring that /test folder. Or maybe ignoring markdown files all together like Federico had mentioned in the initial PR #1082 - think I forgot to put it back into WIP status or something and we hadn't really talked more about the GH Pages thing yet.

Plus, that's why we call it software and not hardware - can be easy to change in the future, if we do it right.

Thoughts?

Tagging @intcreator as well, as it's something to consider as we move toward creating all the tests ever.

Contributor

  • Test(s) exist to ensure functionality and minimize regresstion (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@joshbruce joshbruce requested a review from styfle March 3, 2018 02:33
.editorconfig Outdated
@@ -13,5 +13,5 @@ end_of_line = lf
insert_final_newline = true
trim_trailing_whitespace = true
translate_tabs_to_spaces = true
indent_style = space
indent_style = tab
indent_size = 4
Copy link
Member

Choose a reason for hiding this comment

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

when indent_style is set to tab, it may be desirable to leave indent_size unspecified so readers may view the file using their preferred indentation width

from http://editorconfig.org/#file-format-details

Copy link
Member Author

Choose a reason for hiding this comment

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

Ran into issues with that, I think - most likely because of the asterisk. all files have an indent size of 2...need to override to say something else. We could remove the asterisk, I suppose. Basically saying we only want these specific file types to have these specific qualities. I tried doing something like this, and it didn't seem to work very well - but it could just be my editor + plug-in interaction (tested in both Sublime and VSC, with same unexpected results):

[*]
charset = utf-8
end_of_line = lf
insert_final_newline = true
trim_trailing_whitespace = true

[*.json, *.js]
indent_style = space
indent_size = 2

[*.md, !test/*md]
indent_style = tab

For some reason, when I tried various permutations of this inheritance stack I always got spaces in MD files...?? Could just be something in my environment, though it's not obvious to me what.

Copy link
Member Author

Choose a reason for hiding this comment

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

At one point, I was also tempted to just do this:

[*.json, *.js]
charset = utf-8
end_of_line = lf
insert_final_newline = true
indent_style = space
indent_size = 2

And call it a day...literally only effect JS and JSON. Not sure what would have things like YML though. Also not sure how broad our lint rules go either. Would like to get this to match those as much as possible where possible - just not a deal-breaker if they don't or can't.

.editorconfig Outdated
@@ -13,5 +13,5 @@ end_of_line = lf
insert_final_newline = true
trim_trailing_whitespace = true
translate_tabs_to_spaces = true
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

May be either a deprecated feature - or a Sublime Text specific thing (see first sentence after code): sindresorhus/editorconfig-sublime#12

Automatically converts tabs to spaces on new files. Probably a horrible idea all around anyway. Good catch. ;)

Removed.

@@ -12,6 +12,5 @@ charset = utf-8
end_of_line = lf
insert_final_newline = true
Copy link
Member

Choose a reason for hiding this comment

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

kind of funny that this file doesn't have a new line at the end 😆

Copy link
Member Author

@joshbruce joshbruce Mar 3, 2018

Choose a reason for hiding this comment

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

Dude! I thought that was hilarious. I was literally expecting it to happen as soon as I hit save...so disappointed. 🤣

@joshbruce
Copy link
Member Author

joshbruce commented Mar 3, 2018

Does eslint only do JS? (Revealing my over-dependance on editorconfig right there.)

If so, could literally get rid of the other rules and just apply them to JS files. The objective is to keep developers from failing lint, if their editor can apply the config. Give them the check with the NPM lint:check by @UziTech and a quick way to fix by Federico. And a way to protect the core by indicating with Travis. At that point, we copy-paste from the docs and point them there. Eventually people see it enough and should reduce how often it happens. :)

@joshbruce
Copy link
Member Author

es = EcmaScript

I'm retarded sometimes... https://youtu.be/j9ndS6yu3Kk

@UziTech
Copy link
Member

UziTech commented Mar 3, 2018

by default eslint only lints .js files. You can specify a --ext parameter to add other extensions. .json is a pretty common one that is added

@styfle styfle merged commit 03d0ed0 into markedjs:master Mar 14, 2018
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
* All md files except in test use tab
* Remove translate_tabs_to_spaces
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.

4 participants