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

Separate camelize and classify lookup #118

Merged
merged 1 commit into from
Jun 12, 2020

Conversation

Be-ngt-oH
Copy link
Contributor

What is the purpose of this pull request?

I've noticed a difference between how the lookup for symbols works in a Rails vs a non-Rails context. When in a non-Rails context, the symbol gets camelized, whereas in Rails, we classify it. For example, if given :users, in a non-Rails environment, we're looking for UsersPolicy. In Rails, we'd be looking for UserPolicy.

This PR updates the lookup to allow exact matches in Rails as well, e.g. when you don't want to or can't use Rails inflection. As a side-benefit, this PR makes the difference in behaviour between Rails and non-Rails a bit clearer.

What changes did you make? (overview)

First, we add a new lookup that uses camelize. By renaming the Symbol extension from classify to camelize this maintains the old behaviour in the non-Rails case and allows the exact matching symbols in the Rails case (:users -> UsersPolicy).

Second, we change the existing lookup to check whether classify is available, so it's skipped in the non-Rails case. This second lookup keeps it working in the Rails context (:users -> UserPolicy).

Finally, we've updated the documentation to describe the added behaviour.

PR checklist:

  • Tests included
  • Documentation updated
  • Changelog entry added

docs/lookup_chain.md Outdated Show resolved Hide resolved
@Be-ngt-oH Be-ngt-oH force-pushed the add-camelize-lookup branch 2 times, most recently from 55b680e to 33d78ba Compare June 10, 2020 11:30
@Be-ngt-oH
Copy link
Contributor Author

I have updated the PR to incorporate your ideas. I'd really like to add some tests around the Rails vs non-Rails behaviour but didn't quite manage to do it yet. I have explored two options:

  1. One way is requiring active_support/core_ext/string/inflections at the top. This would allows us to write tests against the classify lookup. However, we cannot do the opposite.
  2. I've tried doing something with a String refinement that just adds classify (calling ActiveSupport::Inflector::classify). The lookup chain is set when the Lookup module is loaded, so you'd need to change that or reload the module. I tried the latter but then didn't manage to undo it without reloading the chain in every single test. I also had trouble scoping the refinement to just one testing class.

The case where you'd have a symbol as a plural and lookup the policy as the singular was not tested previously but I'd really like to add it. I'd be super grateful for ideas on this!

@palkan
Copy link
Owner

palkan commented Jun 10, 2020

We run tests in isolation on CI, so Rails-specific tests (such as railtie_test) do not affect others. You can use rake test:isolated to do the same locally.

You can add one test under test/rails directory (say, test/rails/lookup_chain_test.rb), which loads dummy app first and only then loads test_helper (which in its turn requires action_policy). To make it kinda compatible with rake test, we need to check whether Action Policy has been loaded or not:

# test/rails/lookup_chain_test.rb
require_relative "./dummy/config/environment"

# we can skip this test file altogether if Action Policy has been loaded
return if defined?(ActionPolicy)
require "test_helper"

# or we can reload the lookup chain
# (that would require moving current `self.chain = ...` into a new `reload!` method)
ActionPolicy::LookupChain.reload!

Similarly, the non-Rails test would include return if defined?(Rails) on top of it (if we don't want to run it in the Rails context).

@Be-ngt-oH Be-ngt-oH force-pushed the add-camelize-lookup branch from 33d78ba to c1be553 Compare June 10, 2020 17:45
@Be-ngt-oH
Copy link
Contributor Author

We run tests in isolation on CI, so Rails-specific tests (such as railtie_test) do not affect others. You can use rake test:isolated to do the same locally.

You can add one test under test/rails directory (say, test/rails/lookup_chain_test.rb), which loads dummy app first and only then loads test_helper (which in its turn requires action_policy). To make it kinda compatible with rake test, we need to check whether Action Policy has been loaded or not

Brilliant idea - Thank you very much for that! I was now able to update the branch with tests asserting the new behaviour.

Previously, if you were using ActiveSupport, the lookup would use Rails'
`String#classify` method. Without ActiveSupport it'd use a patched
version which implemented classifying as camelizing a String.

As an example, if the lookup was given a symbol `:users`, it'd behave as
follows:
a) Without ActiveSupport, look for `UsersPolicy`;
b) With ActiveSupport, look for `UserPolicy`;

This change does three things. First, we add a new lookup that uses
`camelize`. This enables the lookup to find `UsersPolicy` when given
`:users` in Rails. This is the new behaviour.

Second, by changing the Symbol extension, we keep the old behaviour in
the non-Rails working.

Finally, we add a new lookup that is only used when `classify` is
available on String and uses that to infer the polict. This ensures that
finding `UserPolicy` still works in the case where we're in a Rails
application.
@Be-ngt-oH Be-ngt-oH force-pushed the add-camelize-lookup branch from c1be553 to 4d7c08f Compare June 12, 2020 12:08
@palkan palkan merged commit 588daef into palkan:master Jun 12, 2020
@palkan
Copy link
Owner

palkan commented Jun 12, 2020

Thanks!

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.

3 participants