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

Rename configuration option blacklistHosts #7622

Merged

Conversation

laiscoolblue
Copy link
Contributor

@laiscoolblue laiscoolblue commented Jun 8, 2020

  • Closes N/A

User facing changelog

Rename configuration option from blacklistHosts to blockHosts

Additional details

  • This PR renames all instances of the configuration option blacklistHosts to blockHosts.
  • This PR also changes all occurrences of the word blacklist throughout the codebase.

How has the user experience changed?

Now when using blacklistHosts, there is an error thrown and the process exits.

Screen Shot 2020-06-26 at 11 43 11 AM

Making it more inclusive by replacing terms that can be perceived as offensive to more self-explanatory names (blacklist/whitelist can reinforce the association of black as bad and white as good).

Examples of projects that already made the change:
Rails
Golang
Graphite Web
HTML

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 8, 2020

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

  • Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
  • Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

If any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'.

User Experience

  • The feature/bugfix is self-documenting from within the product.
  • The change provides the end user with a way to fix their problem (no dead ends).

Functionality

  • The code works and performs its intended function with the correct logic.
  • Performance has been factored in (for example, the code cleans up after itself to not cause memory leaks).
  • The code guards against edge cases and invalid input and has tests to cover it.

Maintainability

  • The code is readable (too many nested 'if's are a bad sign).
  • Names used for variables, methods, etc, clearly describe their function.
  • The code is easy to understood and there are relevant comments explaining.
  • New algorithms are documented in the code with link(s) to external docs (flowcharts, w3c, chrome, firefox).
  • There are comments containing link(s) to the addressed issue (in tests and code).

Quality

  • The change does not reimplement code.
  • There's not a module from the ecosystem that should be used instead.
  • There is no redundant or duplicate code.
  • There are no irrelevant comments left in the code.
  • Tests are testing the code’s intended functionality in the best way possible.

Internal

  • The original issue has been tagged with a release in ZenHub.

@CLAassistant
Copy link

CLAassistant commented Jun 8, 2020

CLA assistant check
All committers have signed the CLA.

@laiscoolblue laiscoolblue force-pushed the rename-blacklisthosts branch from d966bdb to c22541d Compare June 8, 2020 12:57
rafaelcavazin
rafaelcavazin previously approved these changes Jun 8, 2020
maludecks
maludecks previously approved these changes Jun 8, 2020
@jennifer-shehane jennifer-shehane self-requested a review June 8, 2020 14:38
@jennifer-shehane
Copy link
Member

@laiscoolblue Thanks for the contribution! Could you please sign our CLA?

This would be a breaking change, so wouldn't be able to go in as quickly as we may like. We would have to update all docs and some of the on shortcut links also to get this in.

@jennifer-shehane jennifer-shehane added the type: breaking change Requires a new major release version label Jun 8, 2020
@JessicaSachs
Copy link
Contributor

JessicaSachs commented Jun 8, 2020

Hmmm... Can we support both for the near-term so that people can adapt their codebases to use blocklist immediately and then we will deprecate blacklist in the next major version?

@laiscoolblue
Copy link
Contributor Author

@laiscoolblue Thanks for the contribution! Could you please sign our CLA?

This would be a breaking change, so wouldn't be able to go in as quickly as we may like. We would have to update all docs and some of the on shortcut links also to get this in.

The CLA is signed. There's a PR open for the documentation as well: cypress-io/cypress-documentation#2861

Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

We need to rename some files as well.

Also, we can't just mass rename the value - the old value needs to be supported but a deprecation notice must be displayed. We've done this before on previous renamed values. A warning gets displayed to the user to update their code.

We also need to keep it around in the JSON schema definition for cypress.json.

@jennifer-shehane jennifer-shehane dismissed stale reviews from maludecks and rafaelcavazin via 52e68d5 June 9, 2020 04:03
@jennifer-shehane
Copy link
Member

Need to add deprecation warnings but want to wait until the error.js file is cleaned up from #7629

@jennifer-shehane
Copy link
Member

I'll take another look at this Monday now that #7629 is in.

@zenblender
Copy link

Related but for a separate PR: I'd like to see consideration for renaming the "whitelist" Cypress options for cookies to something more appropriate as well, such as "allow list" or "persist list".

@jennifer-shehane
Copy link
Member

I updated this to issue a deprecation warning, but it seems like we will be releasing a breaking version very soon, so I'll have to update the work again back to breaking changes. :P

@zenblender Yes, definitely another issue/PR that we'd be happy to address.

@jennifer-shehane jennifer-shehane changed the base branch from develop to v5.0-release June 23, 2020 05:46
@jennifer-shehane
Copy link
Member

@zenblender Opened a PR for whitelist changes #7782

@jennifer-shehane jennifer-shehane requested a review from flotwig June 23, 2020 05:49
@jennifer-shehane jennifer-shehane dismissed brian-mann’s stale review June 23, 2020 07:57

dismiss brian's earlier review as deprecation warnings have been implemented

@jennifer-shehane jennifer-shehane requested review from chrisbreiding and removed request for jennifer-shehane June 25, 2020 07:33
@chrisbreiding
Copy link
Contributor

I feel like the equivalent to 'blacklist' is 'block' and not 'blocklist', so it should be 'blockHosts'. I also worry folks will think 'blocklist' is a typo or misread it in the docs as 'blacklist' and make the typo themselves, so differentiating it a bit more will help with that.

@jennifer-shehane
Copy link
Member

Think I'm going to rename this to blockHosts after some thought honestly.

@jennifer-shehane jennifer-shehane marked this pull request as draft June 25, 2020 14:37
@jennifer-shehane
Copy link
Member

@chrisbreiding This is funny, I didn't even see your comment above. I had just been thinking about it for days 😆 Glad there's consensus!

@jennifer-shehane jennifer-shehane marked this pull request as ready for review June 26, 2020 05:27
@flotwig flotwig mentioned this pull request Jun 26, 2020
21 tasks
# Conflicts:
#
packages/server/test/support/fixtures/projects/e2e/cypress/integration/r
equest_spec.coffee
@cypress
Copy link

cypress bot commented Jul 11, 2020



Test summary

7885 0 108 0


Run details

Project cypress
Status Passed
Commit f098cd8
Started Jul 11, 2020 8:53 PM
Ended Jul 11, 2020 9:02 PM
Duration 08:38 💡
OS Linux Debian - 10.2
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@jennifer-shehane jennifer-shehane merged commit 27e8c81 into cypress-io:v5.0-release Jul 13, 2020
@Saibamen

This comment has been minimized.

@laiscoolblue laiscoolblue deleted the rename-blacklisthosts branch July 20, 2020 20:54
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 20, 2020

Released in 5.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v5.0.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: breaking change Requires a new major release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants