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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add dot option to treat dots as normal characters #167

Merged
merged 4 commits into from Feb 26, 2021
Merged

feat: add dot option to treat dots as normal characters #167

merged 4 commits into from Feb 26, 2021

Conversation

theoludwig
Copy link
Contributor

@theoludwig theoludwig commented Feb 21, 2021

Hey! 馃憢
Thanks for this CLI tool to use markdownlint. 馃憤

I'm using the CLI tool like this : markdownlint '**/*.md' --ignore node_modules but currently folders like .github are ignored, so my CONTRIBUTING.md, PULL_REQUEST_TEMPLATE.md etc files are ignored.
The current workaround is to use the CLI like this : markdownlint '**/*.md' '.*/**/*.md' --ignore node_modules.

I don't think it is wanted, in my opinion, when we're setting this globbing '**/*.md', it means we want to lint EVERY markdown files in the project, regardless of their location in the folder structure, including folders with a dot.

The fix seems simple, we need to set dot: true in the glob options.
See: https://www.npmjs.com/package/glob#dots

Just to test I edited myself the source files in the node_modules with the fix of this PR, and it is indeed fixing the issue.

@DavidAnson
Copy link
Collaborator

This change makes sense, but I would like to hear from others before making it. The conventional behavior of dot files/directories is to be hidden by default and that's how the tool currently behaves. As you show above, it is already possible to override this behavior if that's what you want.

If we agree to change this behavior, one approach is what you've shown here and another would be to add a new option for this. The advantage of a new option would be that the default behavior does not change and remains consistent with dot file convention - rather than forcing everyone into this new behavior.

@theoludwig theoludwig changed the title fix(glob-options): treat dots as normal characters feat: add dot option to treat dots as normal characters Feb 21, 2021
@theoludwig
Copy link
Contributor Author

I agree, we should avoid breaking changes! 馃槃
Your suggestion of adding a new option is great, so it doesn't force everyone with this new behavior. 馃憤
I just committed with a new boolean option with commander : --dot. @DavidAnson

markdownlint.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@DavidAnson DavidAnson left a comment

Choose a reason for hiding this comment

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

In addition to the minor comments above, there should be two new test cases to verify this behavior. One should verify a dot file and dot directory is ignored without the option and the other should verify the file and directory are included with the option. Thanks!

@theoludwig
Copy link
Contributor Author

Sure, it's always better to add automated tests! 馃槃
I added the two new test cases, but I didn't quite understood what you mean by that :

Also, please sort alphabetically - here and in the list of options in MD and JS.

The options weren't sorted alphabetically, I sorted all the options correctly, feel free to tell me if it's wrong or you thought of something else.

@DavidAnson
Copy link
Collaborator

This looks good, thank you! As I mentioned above, I'd like to give other people a couple of days to have a look and see if they agree. In the meantime, it might be nice to add something like .file-with-dot.md for the new tests you added to confirm that it behaves correctly like the folder does.

@theoludwig
Copy link
Contributor Author

Right, I added .file-with-dot.md. 馃憤
Thanks for your help! 馃槃

I'd like to give other people a couple of days to have a look and see if they agree

Now it's only a new option, and it doesn't affect the old behavior so we could merge this faster, but it's up to you of course! @DavidAnson

Copy link
Collaborator

@DavidAnson DavidAnson left a comment

Choose a reason for hiding this comment

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

Great! Yes, I feel better as an option, and will include this with the upcoming release.

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.

None yet

2 participants