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

Use clearer terminologies #1874

Merged
merged 1 commit into from Nov 16, 2018
Merged

Use clearer terminologies #1874

merged 1 commit into from Nov 16, 2018

Conversation

JuanitoFatas
Copy link
Contributor

@JuanitoFatas JuanitoFatas commented Nov 10, 2018

This Pull Request updates terminology in the codebase:

  • Deprecate Pry.config.exception_whitelist, use Pry.config.unrescued_exceptions instead.
  • Deprecate Pry::DEFAULT_EXCEPTION_WHITELIST, use Pry::DEFAULT_UNRESCUED_EXCEPTIONS instead.

White and black are not clear, use clearer unrescued as suggested.

Original motivation, other community efforts examples:

@0x1eef
Copy link
Contributor

0x1eef commented Nov 10, 2018

i don't think allowlist or whitelist communicates the purpose of this configuration option as clearly as: unrescuable_exceptions. wdyt?

@kyrylo
Copy link
Member

kyrylo commented Nov 10, 2018

Neither https://twitter.com/dhh/status/1032050325513940992 nor
rails/rails#33677 explain the issue with these terms. It says "there's a problem" but it was never specified what exactly is wrong.

I assume that there's some potential issue involving races. I checked https://en.wikipedia.org/wiki/Blacklisting#Origins_of_the_term and didn't see any connections to race. This term is used not only in computing but in other contexts, too.

I guess I am missing something obvious here. I'd appreciate some background.

@0x1eef
Copy link
Contributor

0x1eef commented Nov 10, 2018

@kyrylo
do you think whitelist is a good description for this configuration option though? how is it acting as a whitelist? it is an array of exceptions who Pry::RescuableException considers 'unrescuable' and lets them fall through to the next rescue clause or raise upwards on the call stack. i don't see how "whitelist" applies as a word that describes this.

@0x1eef
Copy link
Contributor

0x1eef commented Nov 10, 2018

i would even think it should not be a configuration option at all but part of a public Pry::RescuableException API, like:

Pry::RescuableException.dont_rescue SomeError

@kyrylo
Copy link
Member

kyrylo commented Nov 10, 2018

I do find this hard to understand:

# Don't catch these exceptions
DEFAULT_EXCEPTION_WHITELIST

I wouldn't be opposed to the name ALLOWED_EXCEPTIONS and Pry.config.allowed_exceptions if that makes people happy. That said, I'm still curious what's wrong with white/black-lists 😁

@0x1eef
Copy link
Contributor

0x1eef commented Nov 10, 2018

i would vote for unrescued_exceptions or something along those lines, allowed_* doesn't communicate what this is for to me.

@0x1eef
Copy link
Contributor

0x1eef commented Nov 10, 2018

ditto on DEFAULT_UNRESCUED_EXCEPTIONS

@0x1eef
Copy link
Contributor

0x1eef commented Nov 10, 2018

'unrescued' seems like the perfect word for this.
https://en.oxforddictionaries.com/definition/unrescued
screenshot 2018-11-10 at 22 26 44

Deprecate Pry.config.exception_whitelist,
use Pry.config.unrescued_exceptions instead.

Deprecate  Pry::DEFAULT_EXCEPTION_WHITELIST,
use Pry::DEFAULT_UNRESCUED_EXCEPTIONS instead.

What white / black means are not clear,
use clearer terminologies.
@JuanitoFatas JuanitoFatas changed the title Use allowlsit where applicable Use clearer terminologies Nov 11, 2018
@0x1eef
Copy link
Contributor

0x1eef commented Nov 12, 2018

lgtm, thoughts @kyrylo ?

@kyrylo
Copy link
Member

kyrylo commented Nov 16, 2018

Nice work, it reads much better now!
P.S. I'm still not sure what's wrong with the old terms though, but whatever, I guess 😄

@kyrylo kyrylo merged commit 4fd7307 into pry:master Nov 16, 2018
@JuanitoFatas JuanitoFatas deleted the update-terminology branch November 17, 2018 14:27
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.

None yet

3 participants