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

Changed parser mode to 'codemod' #850

Merged
merged 3 commits into from Nov 3, 2019
Merged

Changed parser mode to 'codemod' #850

merged 3 commits into from Nov 3, 2019

Conversation

initram
Copy link
Contributor

@initram initram commented Oct 8, 2019

This allows the linter to lint indentation correctly even if the line starts with  

Fixes #120

Though some changes were needed to rules that work on TextNodes as they now behave slightly different. The changes I made to the block indentation rule also made it a bit more consistent in my opinion.

Additional test may be needed to cover what ever corners cases there may be, but all current tests should pass.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I am concerned that this is effectively a breaking change for rules like no-bare-strings (because the whitelist/blacklist there are specifically relying on the fact that the tokens have been swapped). I still think we should do this change, just need to think through how it may be considered a breaking bugfix.

@@ -123,6 +123,7 @@ class Linter {

try {
precompile(stripBom(options.source), {
mode: 'codemod',
Copy link
Member

Choose a reason for hiding this comment

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

👍I absolutely had this package in mind when adding this feature upstream.

@initram
Copy link
Contributor Author

initram commented Oct 11, 2019

Yes, If people had a "&" in their white list and "&" in their template, then it would break for them after this change. But any rule change or fix, may break someones flow.
Also people who have had   in their templates will now get warnings, as this was mapped to whitespace, but it no longer is. We could consider adding more items the the default white list and test that it works for string that are currently being transformed into whitespace.

@rwjblue
Copy link
Member

rwjblue commented Oct 11, 2019

We could consider adding more items the the default white list and test that it works for string that are currently being transformed into whitespace.

Right, I think the main thing we need to ensure is that the items in the current default whitelist that could have been converted from an HTML entity have a corresponding whitelist entry for the entity as well.

@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2019

@initram - Would you mind rebasing? There have been changes to lib/index.js which are causing conflicts, but also the definition of codemod mode changed in @glimmer/syntax@0.43.0 (see glimmerjs/glimmer-vm#983 for details on that breakage WRT codemod mode). We may need more tweaks to accommodate those changes.

initram and others added 3 commits November 3, 2019 12:00
This allows the linter to lint indentation correctly even if the line starts with  
…e strings

I only added the named versions of the HTML entities, and not the numbered ones as they are rarely used.
@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2019

I went ahead and rebased for you, looks like no additional failures crop up so this is good to land!

@rwjblue rwjblue merged commit 549775f into ember-template-lint:master Nov 3, 2019
@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2019

Thank you again @initram!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect indentation on line starting with  
2 participants