Skip to content

Remove helpers from Rails APIs #4

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

Closed
wants to merge 1 commit into from
Closed

Remove helpers from Rails APIs #4

wants to merge 1 commit into from

Conversation

djsegal
Copy link

@djsegal djsegal commented Dec 12, 2015

This is two changes:


see error below
the reason i need to empty @@helpers is because I'm using DeviseTokenAuth and just getting rid of Devise::Controllers::Helpers inside the codebase would still leave that one helper left (which sparks errors)

[1] pry(Devise)> @@helpers
=> #<Set: {Devise::Controllers::Helpers, DeviseTokenAuth::Controllers::Helpers}>

@djsegal
Copy link
Author

djsegal commented Dec 12, 2015

this builds off heartcombo#3714

@twalpole
Copy link
Owner

Can you add testcases?

@@ -325,6 +325,9 @@ def self.add_mapping(resource, options)
mapping = Devise::Mapping.new(resource, options)
@@mappings[mapping.name] = mapping
@@default_scope ||= mapping.name
if defined? Rails and Rails.try(:application).try(:config).try(:api_only)
@@helpers = []
Copy link
Owner

Choose a reason for hiding this comment

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

I don't fully understand why this is needed - but by default @@helpers is a Set - so, if it is necessary to clear it, it's probably better to set to Set.new rather than empty array, it may also be better to not call define_helpers if API rather than clearing @@helpers (If that fixes the issue this is needed for)

@djsegal
Copy link
Author

djsegal commented Dec 12, 2015

@twalpole: i just updated the commit.

  • I dropped the hacky @@helpers = [] because I realized it was something I could fix in a PR to devise_token_auth
  • I added a test that checks for helper methods. I don't know how to go about making an integration test, though

@djsegal
Copy link
Author

djsegal commented Dec 14, 2015

@twalpole sorry about this. reopening this issue. partially resolved by lynndylanhurley/devise_token_auth#469, but still needs the check on helper and helper_method from this PR

@twalpole
Copy link
Owner

@djsegal This branch was merged into the official devise master -- you probably want to issue this PR against that.

@twalpole twalpole closed this Dec 15, 2015
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.

2 participants