Add function-name-arguments-allowed-list #4816

mockey-jockey · 2020-06-01T12:16:33Z

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

Add function-name-arguments-allowed-list #4727

(closes #4727 )

Is there anything in the PR that needs further explanation?

mattxwang

Hey @mockey-jockey, thanks for submitting a PR! I'm also relatively inexperienced with OSS, so don't worry about it at all! Every little bit helps 😄

I can't really comment on whether or not this is the most efficient solution, but right now, you're failing quite a few tests (you can preview the test output on the CI here, or run npm test on your local development environment.

Just taking a quick look, it seems like all of the tests have the message Unknown rule function-name-arguments-whitelist., which is probably being thrown because you haven't added the rule to lib/rules/index.js.

I think if you just add one line to lib/rules/index.js of the form

const rules = {
...
'function-name-arguments-whitelist': importLazy(() => require('./function-name-arguments-whitelist'))(),
...
}

then stylelint will load the new rule that you've made, and you should be good to go! And just to double-check, I'd run npm test locally to make sure that your tests pass before you commit.

mockey-jockey · 2020-06-02T09:21:09Z

@malsf21 thanks it worked, still needs to shape the code here and there. can u pls check once

jeddy3

@mockey-jockey Thanks for making a start on this! Contributions to stylelint are most appreciated and welcome.

Unfortunately, you've gotten off on the wrong foot by using function-url-scheme-whitelist as a blueprint. I suggest using declaration-property-unit-whitelist instead as it's much closer to the functionality of this new rule.

You've picked quite a meaty issue first issue, but everything you need to learn to make this rule work already exists in the declaration-property-unit-whitelist rule. You'll need to adapt some of the code, though, and you'll need to familiarise yourself with the value parser to do this. I suggest having another go and updating this pull request. I'll do my best to feedback on your progress or answer any questions you may have.

Bare with me, though, as reviewing takes time and it may take a while to respond.

jeddy3 · 2020-06-04T17:06:22Z

lib/rules/function-name-arguments-whitelist/index.js

@@ -0,0 +1,64 @@
+// @ts-nocheck


We should use the declaration-property-unit-whitelist as a blueprint rather than function-url-scheme-whitelist one. The latter only accepts an array of whitelisted values, whereas this rule should accept an object of function and parameter pairs (just like declaration-property-unit-whitelist accepts an object of property and unit pairs), .e.g.:

{ "rules": { "function-name-arguments-whitelist": { "url": ["/^images/", "^fonts/"], "blur": ["50%"] } } }

The declaration-property-unit-whitelist looks for units within a declaration's value and compares it to the declaration property. You'll need to adapt the code so that this new rule looks for functions within a declaration's value, and compares the name of the function to the parameters of the function.

mockey-jockey · 2020-06-08T10:12:15Z

hi @jeddy3 , Forgive me for wrongly understood the logic.

As u said have changed the logic based on declaration-property-unit-whitelist blueprint and used value-parser also, still facing some issue.

can you please help me on this, could not able to test and debug the code. That's why I can't able to proceed further with what it's returning a little bit of confusion.

Have updated code and give me some hint on what is the next step.

mockey-jockey · 2020-06-08T20:33:12Z

@jeddy3 can u pls review and help me to proceed further

jeddy3 · 2020-06-10T12:26:58Z

lib/rules/function-name-arguments-whitelist/__tests__/index.js

+
+const { messages, ruleName } = require('..');
+
+testRule({


Your best next step is to change the tests so they align with the intended behaviour of the rule. I suggest starting with:

'use strict'; const { messages, ruleName } = require('..'); testRule({ ruleName, config: [ { brightness: ['50%'], "/^url/": ['/^images/', '/^vendor/'], }, ], accept: [ { code: 'a { filter: blur(20px); }', }, { code: 'a { filter: brightness(50%); }', }, { code: 'a { background: url(images/x.jpg); }', }, { code: 'a { background: url(vendor/x.jpg); }', }, { code: 'a { background: url-x(vendor/x.jpg); }', }, ], reject: [ { code: 'a { filter: brightness(25%); }', message: messages.rejected('brightness', '25%'), line: 1, column: 16, }, { code: 'a { background: url(x.jpg); }', message: messages.rejected('url', 'x.jpg'), line: 1, column: 16, }, { code: 'a { background: url-x(x.jpg); }', message: messages.rejected('url-x', 'x.jpg'), line: 1, column: 16, } ], });

The config is saying:

for the brightness function the parameters can only be 50%

for any function that starts with url the parameters must start with either images or vendor

Then there are accept and reject test cases for this config.

I should validate against the background this contains either base64 string or URL function.

If URL present means have to validate starts with strings

I should validate against the background this contains either base64 string or URL function.

The rule isn't concerned with the type of arguments. It should always treat the arguments as a string for comparsion.

jeddy3 · 2020-06-10T12:34:38Z

lib/rules/function-name-arguments-whitelist/index.js

+				return;
+			}
+
+			valueParser(value).walk((node) => {


Once you have these correct tests in place, you'll need to update the code within this walk function to check the applicable function names against their parameters. You can use the value parser to do this.

checked value parser URL method and got some idea but I don't know to locally test this

but I don't know to locally test this

Paraphasing from the contributing guide.

You can use the interactive testing prompt to run tests for just a chosen set of files (which you'll want to do during development). For example, to run the tests for just the function-name-arguments-whitelist rule:

Run npm run watch to start the interactive testing prompt.

Press p to filter by a filename regex pattern.

Enter function-name-arguments-whitelist

Select the file from the list

This will run the tests for the just this rule on every time you save files.

checked value parser URL method and got some idea

Excellent. Once you have the tests running you can keep trying things out until the tests pass.

mattxwang · 2020-06-28T05:55:15Z

Just as a side comment, I believe the discussion in #4844 and #4845 would change the naming around this rule as well (though I believe nothing has been decided yet). Since this is a new rule, we wouldn't need to do any aliasing/deprecation!

mockey-jockey · 2020-06-29T00:30:43Z

@jeddy3 should I use @malsf21 suggested way or should continue this work pls confirm once.

mattxwang · 2020-06-29T00:39:03Z

I might be wrong here, but it does not look like a final naming convention has been established, and the issue is still in discussion. I wouldn't worry about it for now, and instead focus on implementing the rule and making sure it passes tests - renaming it afterwards won't be too much work!

jeddy3 · 2020-06-29T08:15:59Z

I wouldn't worry about it for now, and instead focus on implementing the rule and making sure it passes tests - renaming it afterwards won't be too much work!

Exactly this.

You can get the tests running using the instructions I wrote above, to be confident the rule is working.

mattxwang · 2020-07-03T07:16:40Z

Just as an update, #4845 is on the cusp of being merged. The priority for this PR should still be to implement the functionality, but we should also change the name of the rule to function-name-arguments-allowed-list to be in line with the new syntax proposed in #4845.

mockey-jockey · 2020-07-15T11:24:31Z

@jeddy3 @malsf21

Resolved test case issue.

Pls, let me know... if anything needs to change mean

mockey-jockey · 2020-07-16T05:59:30Z

@jeddy3 completed the test case and functionality.

All the test cases too passed, can u pls review this whenever u get time. In my project need to add this lint.Thanks

mattxwang

Hi @mockey-jockey,

Was able to take a quick look at your PR, thank you for working hard on updating this. I have a few comments about the documentation and the overall concept of the rule (I'm not sure if you're using functions properly). I am also a pretty junior contributor of the project, so I'm sure I've missed some things.

Take a look at the changes I've requested, and let me know if any of them are unclear / incorrect! Hopefully we can merge this soon fo your project!

mattxwang · 2020-07-17T23:51:01Z

lib/rules/function-name-arguments-allowed-list/README.md

+Given:
+
+```
+["/^images/", "/^http/"]
+```
+
+The following patterns are considered violations:
+
+<!-- prettier-ignore -->
+```css
+a { background-image: url('images/file.jpg'); }
+```
+
+<!-- prettier-ignore -->
+```css
+a { background-image: url('http://www.example.com/file.jpg'); }
+```
+
+<!-- prettier-ignore -->
+```css
+a { background-image: url('data:image/gif;base64,R0lGODlhAQABAIAAAAUEBAAAACwAAAAAAQABAAACAkQBADs='); }
+```


Is this how the rule behaves? You've included images in the allowed-list, but you're saying that background-image: url('images/file.jpg') is a violation.

I would recommend that you match the examples with a portion of the unit tests, so that we are guaranteed that the documentation is always correct.

mattxwang · 2020-07-17T23:51:31Z

lib/rules/function-name-arguments-allowed-list/README.md

@@ -0,0 +1,64 @@
+# function-name-arguments-allowed-list
+
+Specify a whitelist of allowed property and value pairs within declarations.


Per the previous issue, this should be "Specify an list of allowed ...". In addition, this is not a list of allowed property and value pairs - it's a set of function arguments that are allowed within declarations (though that may also be worded better)

mattxwang · 2020-07-17T23:56:07Z

lib/rules/function-name-arguments-allowed-list/README.md

+`object`: `{"unprefixed-function-name": ["/regex/", /regex/] }`
+
+Given:
+
+```
+["/^images/", "/^http/"]


For this portion, it might be clearer to also include the unprefixed-function-name that you're specifying.

can't get this

mattxwang · 2020-07-17T23:59:13Z

lib/rules/function-name-arguments-allowed-list/README.md

+a { background-image: url('data:image/gif;base64,R0lGODlhAQABAIAAAAUEBAAAACwAAAAAAQABAAACAkQBADs='); }
+```
+
+The following patterns are _not_ considered violations:


It would also be good if you explained the behaviour of how this rule interacts with random, unspecified functions (e.g. calc) - and maybe in the tests as well.

can u give me an example for my use case

mattxwang · 2020-07-17T23:59:17Z

lib/rules/function-name-arguments-allowed-list/__tests__/index.js

+
+	config: [
+		{
+			'/^background/': ['/^data:/', '/^images/', '/^http/', '/^vendor/'],


I'm not entirely sure here, but are you saying that background is the function? I believe that url is the actual function that you should be specifying, while background is a CSS property. This indicates either an issue with the tests, or with the implementation of the rule. Not entirely sure about this though, feel free to elabourate if needed.

If background or background-image CSS property has the value called URL function means have to check to start of the folder name.

mockey-jockey · 2020-07-18T03:13:48Z

@malsf21 thanks a lot for spending your time reviewing my PR.

My motive for this rule is:

If background or background-image CSS property has the value called URL function means have to check to start of the folder name.

Pls, let me know if you need further clarification.

mockey-jockey · 2020-07-21T09:34:30Z

@malsf21 any update on this?

mattxwang

Sorry, it's been a busy past few days for me. Thank you for making some of the requested changes!

My motive for this rule is:

If background or background-image CSS property has the value called URL function means have to check to start of the folder name.

Right, that makes sense. However, keep in mind that background itself is not a function - url is. Based off of my understanding from the discussion in #4727 (which could be totally wrong! I'm still new to this project as well 😄 ), my understanding is that you would create a list of allowed arguments for a function.

To put that into perspective, the way function-name-arguments-allowed-list is worded implies that with url, all URLs need to fit the matching arguments, regardless of the CSS property they're attached to (e.g. font-face, background-image, etc.). I understand that's different from the specific use case that you want from the rule, but that's generally how you've implemented the rule as well.

E.g., this would also fail if your allowed-list had "images":

@font-face {
  font-family: "Open Sans";
  src: url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2"),
       url("/fonts/OpenSans-Regular-webfont.woff") format("woff");
}

As an aside, right now you've hard-coded the check for url:

valueParser(value).walk((node) => {
   if (node.value !== 'url') {
    return;
}
...

This means that it won't work with other functions, and won't work as the rule intends. In addition, you'll find that this will force all of your URLs to match your pattern, not just what's in background. Regardless of whichever path you take (for all instances of a function, or instances of a function paired with instances of a CSS property), you'll need to implement this to use the argument.

Does that make sense? I'm not the best technical writer, so please let me know if there's still anything that's unclear.

@jeddy3 feel free to chime in if I've made any mistakes, I might have misinterpreted the spec for the new rule.

mockey-jockey · 2020-07-22T10:12:47Z

No @malsf21, I need to check against background or background-image my rule is defined like that only. I don't know why should I check the font face src maybe I could not understand your question well here. @jeddy3 pls clarify these doubts am going on the right path or not as well @malsf21 comments too.

mockey-jockey · 2020-07-27T09:18:25Z

@jeddy3 @malsf21 any update on this?

jeddy3 · 2020-08-02T10:59:41Z

I need to check against background or background-image my rule is defined like that only.

I understand your use case now. It's quite specific and I can't think of how we can create a general rule that would accommodate it and be suitable as a built-in rule. Fortunately, the plugin system was created for exactly this type of use case.

You can take the code from this pull request and make the small number of changes needed to publish it as a plugin. I suggest you call the plugin stylelint-property-url-allowed-list i.e. specifying a list of allowed URLs for certain properties.

I think we can close this pull request, as the rule is best suited as a plugin.

mockey-jockey · 2020-08-02T11:40:51Z

Hey thanks lot @jeddy3

Can u pls guide me what should I do exactly.

What would be the my next step?

Sorry for the trouble, since am new to the open source contribution couldn't understand lot of things once enter into the style lint learnt lot of things. Thanks for giving me opportunity @jeddy3

Once cleared the test cases issues feel like happy at that time 😆.

Pls let me know what I should I do next ? Your talking about plugin for this is that have any blueprint as u said earlier for this PR

It would very helpful if you give clue about this.

jeddy3 · 2020-08-09T17:22:52Z

What would be the my next step?

You need to create a new plugin using the bulk of the code from this pull request, and then publish that plugin under your own name.

Once you've worked through the process, you're welcome to contribute improvements to the plugin guidelines.

I'm afraid I no longer have much time for open source work and won't be able to guide you further. Good luck, though!

As this will be a plugin rather than a built-in rule, I'm going to close this pull request.

initial commit

3c97e71

mockey-jockey marked this pull request as draft June 1, 2020 12:17

mockey-jockey mentioned this pull request Jun 1, 2020

Add function-name-arguments-whitelist #4727

Closed

mattxwang changed the title ~~initial commit~~ Add function-name-arguments-whitelist Jun 2, 2020

mattxwang requested changes Jun 2, 2020

View reviewed changes

text case fix

a64ee6d

function name argument functionality added

27c276e

mockey-jockey marked this pull request as ready for review June 3, 2020 06:13

mockey-jockey requested a review from mattxwang June 3, 2020 16:35

jeddy3 requested changes Jun 4, 2020

View reviewed changes

jeddy3 added the pr: needs revision label Jun 5, 2020

Changed code logic based on declaration-propety-value-whitlist blueprint

8d63a48

jeddy3 requested changes Jun 10, 2020

View reviewed changes

mattxwang changed the title ~~Add function-name-arguments-whitelist~~ Add function-name-arguments-allowed-list Jul 3, 2020

mockey-jockey and others added 3 commits July 15, 2020 16:05

fixed test case issue

3b3de22

Merge branch 'master' into function-name-arguments-whitelist

a118c99

added test cas fix

28b4213

mockey-jockey added 2 commits July 15, 2020 17:14

changed name to function-name-arguments-allowed-list

907513a

added test case and readme fix

45afff9

mattxwang requested changes Jul 18, 2020

View reviewed changes

mockey-jockey added 2 commits July 18, 2020 08:35

added review comments

2f06e7a

added review comments

e74c8c1

mattxwang requested changes Jul 21, 2020

View reviewed changes

jeddy3 closed this Aug 9, 2020

		@@ -0,0 +1,64 @@
		# function-name-arguments-allowed-list

		Specify a whitelist of allowed property and value pairs within declarations.

Add function-name-arguments-allowed-list #4816

Add function-name-arguments-allowed-list #4816

Conversation

mockey-jockey commented Jun 1, 2020 • edited by mattxwang

mattxwang left a comment

Choose a reason for hiding this comment

mockey-jockey commented Jun 2, 2020

jeddy3 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockey-jockey commented Jun 8, 2020 • edited

mockey-jockey commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattxwang commented Jun 28, 2020

mockey-jockey commented Jun 29, 2020

mattxwang commented Jun 29, 2020

jeddy3 commented Jun 29, 2020

mattxwang commented Jul 3, 2020

mockey-jockey commented Jul 15, 2020 • edited

mockey-jockey commented Jul 16, 2020

mattxwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockey-jockey commented Jul 18, 2020

mockey-jockey commented Jul 21, 2020

mattxwang left a comment • edited

Choose a reason for hiding this comment

mockey-jockey commented Jul 22, 2020

mockey-jockey commented Jul 27, 2020

jeddy3 commented Aug 2, 2020

mockey-jockey commented Aug 2, 2020

jeddy3 commented Aug 9, 2020

mockey-jockey commented Jun 1, 2020 •

edited by mattxwang

jeddy3 left a comment •

edited

mockey-jockey commented Jun 8, 2020 •

edited

mockey-jockey commented Jul 15, 2020 •

edited

mattxwang left a comment •

edited