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

Remove unused variables #58

Merged
merged 2 commits into from
Aug 10, 2020
Merged

Remove unused variables #58

merged 2 commits into from
Aug 10, 2020

Conversation

joelhawksley
Copy link
Contributor

@joelhawksley joelhawksley commented May 13, 2020

👋 hello, friends!

In the latest release, I'm getting the following warnings when running the https://github.com/github/view_component test suite:

/Library/Ruby/Gems/2.6.0/gems/better_html-1.0.14/lib/better_html/test_helper/safe_erb_tester.rb:55: warning: assigned but unused variable - tester
/Library/Ruby/Gems/2.6.0/gems/better_html-1.0.14/lib/better_html/tokenizer/base_erb.rb:52: warning: assigned but unused variable - token

These warnings appear to be valid. The test suite does pass without these assignments.

It would be lovely if a release could be cut with these changes ❤️ Let me know if there's anything I can do to help!

When running better-html with verbose output,
I get the error:

/Library/Ruby/Gems/2.6.0/gems/better_html-1.0.14
/lib/better_html/test_helper/safe_erb_tester.rb:55: 
warning: assigned but unused variable - tester
Before this change, I get the warning:

/Library/Ruby/Gems/2.6.0/gems/better_html-1.0.14
/lib/better_html/tokenizer/base_erb.rb:52: warning: 
assigned but unused variable - token
@vinistock
Copy link
Member

I think that we can also get rid of the token local variable in add_token. We weren't using it, so it's probably safe to remove it and just append to the array straight away.

I tested this out for our CI and it passed. @EiNSTeiN- thoughts?

Copy link
Contributor

@thegedge thegedge left a comment

Choose a reason for hiding this comment

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

We just opened #63 and missed this PR completely. Would love to get this in to reduce warnings on another internal tool.

@thegedge thegedge merged commit 1656e5c into Shopify:master Aug 10, 2020
@joelhawksley joelhawksley deleted the patch-1 branch August 10, 2020 16:05
@maxbeizer
Copy link

@tomstuart / @thegedge Looks like this got approved and merged in August but the last release was in May. Would y'all be amenable to cutting a new release with this included? 🙇 ✨ 🍰

@tomstuart tomstuart mentioned this pull request Jan 26, 2021
@tomstuart
Copy link
Contributor

@maxbeizer I’ve opened #64 to do this. The build has stopped working so I’ll fix that first and then get the release out.

@maxbeizer
Copy link

✨ Thank you @tomstuart 🍰 🚀 🎊

@tomstuart
Copy link
Contributor

v1.0.16 is now released with this change 🎉

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.

5 participants