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

fix: change keyword plugin and import regexp #3668

Merged
merged 3 commits into from
Nov 29, 2021
Merged

fix: change keyword plugin and import regexp #3668

merged 3 commits into from
Nov 29, 2021

Conversation

iChenLei
Copy link
Member

@iChenLei iChenLei commented Nov 18, 2021

Try to fix #3660

We discovered that the regex used for import and plugin have a ? at the end of them, which means @plugi and @impor work as well as @plugin and @import.
These are defined here: https://github.com/less/less.js/blob/master/packages/less/src/less/parser/parser.js#L1687 and https://github.com/less/less.js/blob/master/packages/less/src/less/parser/parser.js#L1847.
This doesn't look like an intentional decision and more of a regex mistake. See here for example inputs: https://regex101.com/r/NUw7E2/2
Is this correct? --from @edhgoose

const dir = parserInput.$re(/^@import?\s+/);

const dir = parserInput.$re(/^@plugin?\s+/);

ready for code review, cc @matthew-dean . btw, i add .DS_Store into .gitignore

  • code complete
  • unit test

Copy link
Contributor

@edhgoose edhgoose left a comment

Choose a reason for hiding this comment

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

This looks correct to me - thank you.

I'm not sure if a test already exists for this, but perhaps another test to confirm that the plugin and import must be proceeded by the @ symbol and followed by a space might be a good idea? I don't think I can see tests that prove this?

Same for perhaps ensuring that they're at the beginning of a line too? This may be going too far, but might help if anyone changes these regexes in the future.

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.

@plugi and @impor are supported
3 participants