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

Change name of "whitelist" options to "exclude" options #72

Closed
rootwork opened this issue Jan 12, 2021 · 5 comments
Closed

Change name of "whitelist" options to "exclude" options #72

rootwork opened this issue Jan 12, 2021 · 5 comments

Comments

@rootwork
Copy link

It's confusing that the white_listed_ inputs for URLs, patterns, and files excludes things rather than including them; usually you'd use the term "whitelist" to describe things you are explicitly including that would otherwise not be included.

Additionally, in the output "whitelist" is misspelled for the URL option:

url whitetlist: []

My suggestion would be to change these three input options to exclude_urls, exclude_patterns and exclude_files, to match the include_files option that already exists. (And also update those values in the output itself.)

If you don't want to break existing configs, you could leave the white_listed_ options as valid along with the new ones, but only list the latter in the docs.

@vsoch
Copy link
Collaborator

vsoch commented Jan 12, 2021

Whitelist means that we are adding them to the list of checked/good, so they basically aren't checked. You are correct that you could read it the other way around and say that it's something being excluded (not checked). That's an interesting idea to add exclude_files along with whitelist (so older versions do not break). If you'd like to open a pull request with these changes I'd be happy to review - we could even just make the changes for the action without editing the client, as likely most users will be interacting with just the client. @SuperKogito what are your thoughts?

We can definitely fix up the spelling error asap, I'll open a PR shortly.

@vsoch
Copy link
Collaborator

vsoch commented Jan 12, 2021

@rootwork I don't see where the whitetlist spelling mistake is in this repository - if you want to include this fix with your PR maybe that would be a cleaner way to get all the changes in at once!

@SuperKogito
Copy link
Member

That is an interesting point; both rationals for exclude or whitelist are compelling. As @vsoch explained, the initial idea was to allow the user to skip certain links during checks.
In my opinion, for the sake of consistency (since we use include_files) it makes more sense to use exclude and -without being political- we might want to avoid the term whitelisted, or at least since we have a good alternative, we can use use exclude. @vsoch this is just my opinion but I trust your judgement on this.

@vsoch
Copy link
Collaborator

vsoch commented Jan 13, 2021

I agree @SuperKogito, and after thinking over it, I think we should just deprecate the previous white_listed_* variables. I have a PR for both of you to review with these changes, here #73.

@rootwork I still can't find that spelling mistake you are referencing...

@vsoch
Copy link
Collaborator

vsoch commented Jan 13, 2021

Okay, the changes are done here, #73 ! If you find the spelling error, please open another issue or PR with a link or the fix. Otherwise, thanks for opening the issue! Closing as we've resolved.

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

No branches or pull requests

3 participants