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

FR: add linter-ignore-start,linter-ignore-end #701

Closed
Leiyi548 opened this issue May 2, 2023 · 22 comments · Fixed by #725
Closed

FR: add linter-ignore-start,linter-ignore-end #701

Leiyi548 opened this issue May 2, 2023 · 22 comments · Fixed by #725
Assignees
Labels
enhancement New feature or request resolution/update-made A change has been made that should resolve this issue or request rule suggestion Suggestion to add or edit a rule

Comments

@Leiyi548
Copy link

Leiyi548 commented May 2, 2023

Is Your Feature Request Related to a Problem? Please Describe.

I have some special css fragments, syntax,I have some special css fragments, syntax.

I don't want this area to be formatted

Describe the Solution You'd Like

A clear and concise description of what you want to happen.

Please include an example where applicable:

<!-- linter-ignore-start -->
                          This area will not be formatted
<!-- linter-ignore-end -->
<!-- linter-ignore-start -->
                          This area will not be formatted
<!-- linter-ignore-end -->```

## Describe Alternatives You've Considered

A clear and concise description of any alternative solutions or features you've considered.

## Additional Context

Add any other context or screenshots about the feature request here.

you can see https://prettier.io/docs/en/ignore.html#range-ignore
@Leiyi548 Leiyi548 added the rule suggestion Suggestion to add or edit a rule label May 2, 2023
@pjkaufman
Copy link
Collaborator

This is definitely an interesting suggestion. I am not sure if this will be added given how much of an overhaul it would take to accommodate this kind of setup with how the current ignoring of values works. However it may be worthwhile to reevaluate what would need to be done once #334 gets implemented.

@pjkaufman
Copy link
Collaborator

Could you provide an example block that you do not want linted? I am not sure that the ignore start and end makes the most sense to fix the problem you are hitting, but it is likely more versatile.

@pjkaufman
Copy link
Collaborator

I have been thinking about this and I will see about adding this for the next release. This is a much simpler way of allowing custom ignore than I initially thought up for doing it (I have a bad habit of trying to over complicate things). Hopefully I can tackle it some time this coming week or sooner.

@pjkaufman
Copy link
Collaborator

@Leiyi548 , for non-pasting rules, would it also be helpful to be able to turn off a specific rule if only 1 is causing a problem?

I am guessing that this is something that would be desired, but I want to check here before I proceed with the regular custom ignore changes so they can go out together if possible. Right now, barring any issues with custom ignore, I just need to add documentation around the global custom ignore.

@pjkaufman
Copy link
Collaborator

pjkaufman commented May 17, 2023

My previous comment would mean that each rule would look for its own alias as part of the range ignore to see if it needed to be disabled.
So something like:

<!-- linter-ignore-remove-multiple-blank-lines-start -->


                          This area will not be formatted by multiple blank lines


<!-- linter-ignore-remove-multiple-blank-lines-end -->

@Leiyi548
Copy link
Author

@pjkaufman It's awesome

@pjkaufman
Copy link
Collaborator

@Leiyi548 , @chrisgrieser what do you think of the above? Any caveats that you can think of or would it not be useful?

I can see using the rule name in particular being useful, but also potentially causing problems for the aliases as they do change every once and a while (i.e. when a typo is present.).

@Leiyi548
Copy link
Author

@pjkaufman No Problem

@pjkaufman
Copy link
Collaborator

pjkaufman commented May 17, 2023

Also, should the matching be exact or allow some leeway? For example allow multiple spaces inside the html comment or more than 2 dashes in the html comment?

For example <!----- linter-ignore-start --> or <!-- linter-ignore-start --> or <!---- linter-ignore-start -----------------> are just variations of it.

@Leiyi548
Copy link
Author

@pjkaufman allow some leeway.

@pjkaufman
Copy link
Collaborator

Gotcha. I can update it to allow some leeway. I will require that it be a single html comment though for each indicator.

@pjkaufman
Copy link
Collaborator

The logic is currently mostly in place for the range for turning all linter rules on and off. The one for specific rules may need to be manually tested, but hopefully will pose no more of an issue than global range ignore.

@pjkaufman
Copy link
Collaborator

I will likely review my first change for this to add global range indicators and get that merged. Once that is done, I will start on the rule specific range ignores.

@chrisgrieser
Copy link
Contributor

chrisgrieser commented May 17, 2023

I like the idea for this feature.

For consistency with how other linters implement the same feature, I'd suggest maybe using the same wording as them?

For example, eslint uses enable and disable followed by a comma-separated list of rule names.

One thing to note here is also that the rule aliases actually cannot be found anywhere in the linter plugin settings, you have to follow the links to the docs. Adding the aliases to the (already quite crowded) linter settings page would work, but I think it could make more sense to add them in a more accessible way. Maybe inserting them via completion similar to tags?

@pjkaufman
Copy link
Collaborator

I am fine with using enable and disable in place of start and end.

It would be nice to be able to allow text completion, but I am not entirely sure how easy that is in Obsidian. If it is simple in Obsidian that should be doable, but it it requires a lot of UI work then it may not be feasible at least not initially.

@pjkaufman
Copy link
Collaborator

I like the idea for this feature.

For consistency with how other linters implement the same feature, I'd suggest maybe using the same wording as them?

For example, eslint uses enable and disable followed by a comma-separated list of rule names.

One thing to note here is also that the rule aliases actually cannot be found anywhere in the linter plugin settings, you have to follow the links to the docs. Adding the aliases to the (already quite crowded) linter settings page would work, but I think it could make more sense to add them in a more accessible way. Maybe inserting them via completion similar to tags?

@chrisgrieser , would it make sense to add a command that inserted start and end ignores for a rule or rules at the current cursor or around the selected text? I could possibly add something like that. That way the user would not need to know the aliases and could interact directly with the UI to get ignore blocks added.

I am still thinking about the comma separated rules options for rule names. I really am not a fan of parsing out which rules are disabled at a specific block since that will require each rule to check if the ignore block applies to them instead of just finding what applies to them from the get go, but that would be possible.

@pjkaufman
Copy link
Collaborator

It could also be a multiselect with a preview of what the source code would look like post insertion.

Do note that the linter disabling syntax does not care about how it is found so long as it is present, so the value being found in a code block or inline code or math any kind of math block will result in it getting triggered.

@pjkaufman
Copy link
Collaborator

Looks like suggestions could be done like how they are done via this plugin here, but that will require work in CodeMirror: https://github.com/tth05/obsidian-completr/blob/master/src/main.ts

@chrisgrieser
Copy link
Contributor

would it make sense to add a command that inserted start and end ignores for a rule or rules at the current cursor or around the selected text? I could possibly add something like that. That way the user would not need to know the aliases and could interact directly with the UI to get ignore blocks added.

oh, that's even better.

I am still thinking about the comma separated rules options for rule names. I really am not a fan of parsing out which rules are disabled at a specific block since that will require each rule to check if the ignore block applies to them instead of just finding what applies to them from the get go, but that would be possible.

yeah, thinking about it, having the option to disable/enable specific rules is maybe a bit overkill yeah 😅

Just disabling/enabling the linter should be totally useful on its own, yes

@pjkaufman
Copy link
Collaborator

would it make sense to add a command that inserted start and end ignores for a rule or rules at the current cursor or around the selected text? I could possibly add something like that. That way the user would not need to know the aliases and could interact directly with the UI to get ignore blocks added.

oh, that's even better.

I am still thinking about the comma separated rules options for rule names. I really am not a fan of parsing out which rules are disabled at a specific block since that will require each rule to check if the ignore block applies to them instead of just finding what applies to them from the get go, but that would be possible.

yeah, thinking about it, having the option to disable/enable specific rules is maybe a bit overkill yeah 😅

Just disabling/enabling the linter should be totally useful on its own, yes

I am definitely fine with having a single comment per rule to disable, but that would get tedious for users. So it is definitely a balancing act.

@pjkaufman
Copy link
Collaborator

Looks like adding the ability to do autocomplete would require an EditorSuggest which will require CodeMirror code. This is possible, but will likely take more time than I have before I would like to do this next release. As such, I will break this feature change into 2 parts:

  • This issue will deal with adding custom ignore for all rules
  • The other that I will create will deal with rule specific ignoring

@pjkaufman
Copy link
Collaborator

The custom ignore syntax should go out in the next release. The logic for ignoring specific rules will need to be addressed in a later ticket. I will have to look into rule specific ignores down the road. In the meantime, hopefully this will take care of most use cases for now.

@pjkaufman pjkaufman added the resolution/update-made A change has been made that should resolve this issue or request label May 21, 2023
@pjkaufman pjkaufman self-assigned this May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request resolution/update-made A change has been made that should resolve this issue or request rule suggestion Suggestion to add or edit a rule
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants