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

Add url secondary option to any rules #7743

Merged
merged 13 commits into from
Jun 17, 2024
Merged

Conversation

emmacharp
Copy link
Contributor

@emmacharp emmacharp commented Jun 6, 2024

Which issue, if any, is this issue related to?

Closes #7484

Is there anything in the PR that needs further explanation?

Based on @ybiquitous implementation proposal here. Additional changes were made to lintSource.mjs and validateOptions.mjs as to prevent undefined errors and invalid option warnings. customUrls property was added to checkAgainstRule.mjs as to pass certain tests.

Copy link

changeset-bot bot commented Jun 6, 2024

🦋 Changeset detected

Latest commit: 8170609

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@emmacharp emmacharp changed the title Custom urls Implement custom urls Jun 6, 2024
@ybiquitous
Copy link
Member

@emmacharp Thanks for the pull request! First of all, you can fix the system test failures by running the command:

npx jest system-tests --updateSnapshot

@ybiquitous ybiquitous changed the title Implement custom urls Add url secondary option to any rules Jun 7, 2024
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

[suggest] We should add a new section for url under the rules section in the documentation:
https://github.com/stylelint/stylelint/blob/9a2e6540f92509628df76032262293ae57d32dda/docs/user-guide/configure.md#rules

lib/utils/report.mjs Outdated Show resolved Hide resolved
@ybiquitous
Copy link
Member

[ask] @emmacharp Just to be sure, how will you specifically configure your rules when this url option is released?
I'm just wondering if you need the function support as described on #7484 (comment).

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@emmacharp
Copy link
Contributor Author

@ybiquitous, I do not see the need for function support as of now.

@emmacharp
Copy link
Contributor Author

npx jest system-tests --updateSnapshot

This command is giving me errors. For instance:

 SyntaxError: Cannot use import statement outside a module
   at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1505:14)

Any idea why?

@ybiquitous
Copy link
Member

Hum, I've never seen this error SyntaxError: Cannot use import statement outside a module ... 🤔

I might be wrong, but it seems due to environmental issues. Here's my environment (macOS latest)

$ node -v && npm -v
v22.2.0
10.8.1

And the clean installation and updating Jest snapshots work for me.

$ npm ci
...
$ npx jest system-tests --updateSnapshot
...
$ git status
On branch custom_urls
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   system-tests/002/__snapshots__/fs.test.mjs.snap
	modified:   system-tests/002/__snapshots__/no-fs.test.mjs.snap
	modified:   system-tests/003/__snapshots__/fs.test.mjs.snap
	modified:   system-tests/003/__snapshots__/no-fs.test.mjs.snap
	modified:   system-tests/004/__snapshots__/fs.test.mjs.snap
	modified:   system-tests/004/__snapshots__/no-fs.test.mjs.snap
	modified:   system-tests/005/__snapshots__/commonjs.test.cjs.snap

no changes added to commit (use "git add" and/or "git commit -a")

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

[suggest] Can you add a new test file named lib/__tests__/standalone-custom-url.mjs to test this new feature? If you prefer another file name, you may take it.

docs/user-guide/configure.md Outdated Show resolved Hide resolved
@emmacharp
Copy link
Contributor Author

emmacharp commented Jun 10, 2024

I've been able to run jest by following the second option here:
https://jestjs.io/docs/ecmascript-modules

@ybiquitous
Copy link
Member

Oh, I forgot about --experimental-vm-modules 😅

"test": "node --experimental-vm-modules node_modules/jest/bin/jest.js",

So, we should run Jest like this (of course, NODE_ENV is also available):

npm run test --ignore-scripts -- --updateSnapshot system-tests/

@emmacharp
Copy link
Contributor Author

[suggest] Can you add a new test file named lib/__tests__/standalone-custom-url.mjs to test this new feature? If you prefer another file name, you may take it.

I'm not really familiar with these kinds of test but I'll give it a shot and report back!

@ybiquitous
Copy link
Member

This test file might be helpful: 👇🏼
https://github.com/stylelint/stylelint/blob/3bdb897f647b776973abd630114141f96990c2e6/lib/__tests__/standalone-quiet.test.mjs

If you find it too hard, feel free to ask for help. If you don't mind, I can also do that instead of you. 😃

@emmacharp
Copy link
Contributor Author

Reporting back!

I've created a test file here which seems to work as intended. Still new to Jest so I hope it really is. Hehe.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM 👍🏼

@Mouvedia
Copy link
Member

@ybiquitous, I do not see the need for function support as of now.

The only use case that I can see would be to dynamically change the anchor/fragment.
This can be postponed.

@ybiquitous ybiquitous merged commit 4000bfd into stylelint:main Jun 17, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add url secondary option to any rules
3 participants