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

Rubocop and comment out unneeded gems #29

Merged
merged 4 commits into from
Aug 25, 2017
Merged

Conversation

ndushay
Copy link
Contributor

@ndushay ndushay commented Aug 24, 2017

Resolves #22

Also commented out gems related to html or js or css or windows, as this is going to be json or xml responses only. Happy to move that commit to a separate PR.

We can twiddle our rubocop settings if these initial ones prove annoying ... or nominate your favorites in the comments!

@coveralls
Copy link

Coverage Status

Coverage increased (+1.09%) to 57.5% when pulling fa45c7b on rubocop into beb3779 on show-action-naomi.

@ndushay ndushay changed the title [WIP] Rubocop and comment out unneeded gems Rubocop and comment out unneeded gems Aug 24, 2017
@@ -15,16 +15,16 @@ gem 'sqlite3'
# Use Puma as the app server
gem 'puma', '~> 3.7'
# Use SCSS for stylesheets
gem 'sass-rails', '~> 5.0'
# gem 'sass-rails', '~> 5.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just delete instead of comment out?

Copy link
Contributor Author

@ndushay ndushay Aug 25, 2017

Choose a reason for hiding this comment

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

Excellent question! All of this stuff was added by the rails generator, including pinning to versions. I was feeling tentative, so I commented stuff out. I would be happy to remove cruft if we prefer -- we can always get it back from git history or another rails app if we need it. What do you think?

See also #30 which is about removing directories and the like for unused stuff -- are we comfy with removing all this stuff?

Copy link
Contributor

Choose a reason for hiding this comment

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

My opinion on this one is that we might as well remove this stuff from the Gemfile. These are fairly small changes that would be easy to reinstate, even just by - as you say - running the rails generator again to figure out what we're missing.

I'm less certain about #30 - I believe that removing some of those directories can come back to bite us later and it might be worth just leaving them in there?

@sul-dlss sul-dlss deleted a comment from coveralls Aug 25, 2017
@tallenaz tallenaz merged this pull request into show-action-naomi Aug 25, 2017
@tallenaz tallenaz deleted the rubocop branch August 25, 2017 18:51
@ndushay ndushay restored the rubocop branch August 25, 2017 20:16
@ndushay ndushay mentioned this pull request Aug 25, 2017
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

5 participants