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

Introduce markdownlint #864 #2846

Merged
merged 8 commits into from
May 6, 2019
Merged

Introduce markdownlint #864 #2846

merged 8 commits into from
May 6, 2019

Conversation

roottool
Copy link
Contributor

Description

I introduced markdownlint in Boostnote.
markdownlint

Note

This feature checks all rules and aliases.
I didn't implement ignore settings on purpose.
Because I think that it will exceed the range dealt with in #864.

Issue fixed

#864

Type of changes

  • ⚪ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • 🔘 Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • ⚪ I have written test for my code and it has been tested
  • 🔘 All existing tests have been passed
  • 🔘 I have attached a screenshot/video to visualize my change if possible

@guardrails
Copy link

guardrails bot commented Jan 29, 2019

No more findings on this branch.
This means you fixed everything we detected earlier. Good job!! 👍🎉

We will keep this comment up-to-date as you go along and notify you of any security issues that we identify.

Happy with the results? Give your feedback.

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Jan 30, 2019
'getAnnotations': validatorOfMarkdown,
'async': true
},
mode: 'markdown',
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use markdown mode here. We set the mode according to the props or self-detecting language result if the CodeEditor is used in snippet note.
https://github.com/BoostIO/Boostnote/blob/885f656d3458af090c62f81b4f8a1c775effcdc6/browser/components/CodeEditor.js#L357-L361

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood.
I fixed it. Please check it again.

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Feb 1, 2019
Copy link
Contributor Author

@roottool roottool left a comment

Choose a reason for hiding this comment

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

I took a mistake.
CodeEditor in Snippet Note was linted in all languages by markdownlint.
mistake

This fix is for Snippet Note.
So I added a logic to check Snippet Note using Markdown and Markdown Note.

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. needs extra review 🔎 Pull request requires review from an additional reviewer. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Feb 5, 2019
@ZeroX-DG ZeroX-DG requested a review from Rokt33r February 5, 2019 01:27
@ZeroX-DG
Copy link
Member

ZeroX-DG commented Feb 5, 2019

@Rokt33r do we need a setting for this?

@Rokt33r
Copy link
Member

Rokt33r commented Feb 5, 2019

@ZeroX-DG I think so. 😄

@roottool
Copy link
Contributor Author

We need writing config.json or create a new settings form in interface settings(red circle of below image) to set markdownlint settings.
boostnotesettings

But probably I can't do it for a while.
Because I will be busy from next week for my private.

And if Implementing markdownlint settings includes this PR, I think it is difficult to test because the coverage is too wide.
After resolving the implementation of markdownlint, I think it is a good idea to create a new issue.

@Rokt33r Rokt33r removed needs extra review 🔎 Pull request requires review from an additional reviewer. approved 👍 Pull request has been approved by sufficient reviewers. labels Apr 28, 2019
@Rokt33r Rokt33r added this to the v0.11.15 milestone Apr 28, 2019
@Rokt33r Rokt33r added approved 👍 Pull request has been approved by sufficient reviewers. awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Apr 28, 2019
@Rokt33r
Copy link
Member

Rokt33r commented Apr 28, 2019

@roottool Could you resolve the conflicts?

@roottool
Copy link
Contributor Author

@Rokt33r Sure! I resolved the conflicts.
Please check the code again.

@Rokt33r Rokt33r merged commit 3779016 into BoostIO:master May 6, 2019
@Rokt33r Rokt33r removed approved 👍 Pull request has been approved by sufficient reviewers. awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels May 6, 2019
@roottool roottool deleted the IntroduceMarkdownLint#864 branch May 6, 2019 12:26
@kleinjm
Copy link

kleinjm commented May 22, 2019

How do you disable this?

@judsoniv
Copy link

It would be great to get those preferences. I find the it distracting to see the errors for things I do not care about for my purposes, such as using hard tabs or line length. And it seems to be on by default and not able to be disabled? I don't think it would have expanded the scope too much to just have a stub on-off toggle in preferences until the full lint-preferences section was ready.

@roottool
Copy link
Contributor Author

I'm working on it to be able to edit the markdownlint rule at #2983.
markdownlint is turn off by json.
Therefore, I had to add a new config editor in preferences of Boostnote.

@niorko
Copy link

niorko commented May 22, 2019

This feature bothers me a lot, how can I disable it? :) Thanks

@Zorlin
Copy link

Zorlin commented May 23, 2019

In the same boat... did not expect this change and it's really annoying with seemingly no easy way to disable it.

@takacsot
Copy link

I just got the fresh update and this feature is F^#$ing annoying.

And there is not option to switch off!!!!

I am almost exclusively using the editor mode and those warnings are sh@#t.

@judsoniv
Copy link

roottool: It's not fun to get negative feedback on a feature. I want to thank you for contributing to the Boostnote product. This feature, once more mature, will be one that I will use and enjoy. I thought I would take a minute to expand on my pervious comment with my thoughts after using this for a day.

Here's what I think would make this feature prime-time for me:

  • Toggle in preferences to turn the entire feature on or off
  • Toggles in preferences to enable or disable the individual markdownlint rules
  • Warning icon should be on the line that has the error, rather than the next line

Again, I appreciate shipping software, and I understand that you're contributing to this project because you want to, not because you have to. You try to do something nice for people, and users like me yell at you. Sorry about that.

I look forward to your next iteration.

@bkj
Copy link

bkj commented May 23, 2019

Any way to disable? Or if not, is there an easy way to roll back my version?

@jeffisabelle
Copy link

Hi @roottool,

Thanks for your work on this, I'm pretty sure hearing complaints about your opensource contributions sucks, please consider this as constructive feedback.

It's probably nice to have a markdown linter for many cases but for my use case, it somewhat breaks all the fun I have with boostnote. I tend to store a lot of logs under a code snippet, now it throws a bunch of linter exceptions. I actually don't care about the markdown linting on this case. It'd be great if we can have a toggle in preferences to disable linting.

Also, I noticed linting makes the switch between editor mode and preview mode a lot slower.

For people who have similar problems, I rolled back to the previous version manually by installing it from here;
https://github.com/BoostIO/boost-releases/releases/download/v0.11.15/Boostnote-mac.zip

@Zorlin
Copy link

Zorlin commented May 29, 2019

It looks like the latest update to Boostnote no longer has this feature enabled by default. If you liked it, you can go to Preferences -> Interface -> Enable MarkdownLint.

I would like to take the time to say thank you for building this, even if I didn't like it - as an optional feature I think it's great.

I think making it opt-in by default was a great decision too.

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.

10 participants