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

Warn when replacing placeholders #219

Merged
merged 1 commit into from
Jan 21, 2020

Conversation

graemeboyd
Copy link
Contributor

This PR addresses an issue we've faced with a large Turnip codebase using placeholders with generic names like :object in multiple helpers across multiple files. This led to the placeholders being easily overridden and producing obscure and hard to track down bugs in our test suite.

The main change is to issue a warning that a a placeholder is being replaced if there's already a non-default placeholder. The warning includes the file and line using caller_locations which is supported in Ruby 2.2 onwards.

With just this change the Turnip test suite now issues warnings as it runs as it relies on replacing placeholders being silent. To address this I've added a .clear convenience method which removes all the added placeholders reverting them to the original default.

@graemeboyd graemeboyd changed the title Disallow redefining placeholders Warn when replacing placeholders Jan 8, 2020
@@ -19,6 +20,10 @@ def find(name)
placeholders[name] or default
end

def clear
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO adding a new public API to eliminate warnings doesn't make sense to me.
How about define the clear method in a Refinement and using it in the test instead?

@gongo What do you think?

# spec/support/clear_place_holders
module ClearPlaceHolders
  refine(Turnip::Placeholder) do
    def clear
      placeholders.clear
    end
  end
end

# spec/placeholder_spec.rb
using ClearPlaceHolders

describe Turnip::Placeholder do
  after do
    described_class.clear
  end
  # ...
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hanachin Thanks! I think it's good
@graemeboyd I don't think it's good to make it Public API if Placeholder.clear just code for testing.

lib/turnip/placeholder.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@gongo gongo left a comment

Choose a reason for hiding this comment

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

@graemeboyd

Thanks PR !! I'm sorry that the confirmation was delayed.
Could you confirm my comment.

If a placeholder is replaced a warning is now emitted. As the test suite
relied on placeholder replacements being silent, placeholders are now
cleared to defaults after each test case. Internal calls made from dsl.rb are filtered
out in order to show the real location of the replacement.
@graemeboyd graemeboyd force-pushed the disallow-redefining-placeholders branch from f36255e to 70bb287 Compare January 20, 2020 09:39
@graemeboyd
Copy link
Contributor Author

Hi @gongo and @hanachin, sorry it's taken me a while to continue this. I hadn't realised clearing the placeholder hash by using send was already used elsewhere in the test suite. I've changed the PR to remove introducing a new public method and I've squashed the commits down.

Copy link
Contributor

@hanachin hanachin left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@gongo
Copy link
Collaborator

gongo commented Jan 21, 2020

@graemeboyd @hanachin Thanks for good change and comments!

@gongo gongo merged commit 199485c into jnicklas:master Jan 21, 2020
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