mongoid / mongoid-grid_fs

A pure Mongoid/Moped implementation of the MongoDB GridFS specification.
Other
83 stars 50 forks source link

Rubocop and Travis CI updates. Remove the Danger gem. #69

Closed jonatack closed 6 years ago

jonatack commented 6 years ago

This gem looks like it needs a little update love :heart: but first we need to get past all the new Rubocop rules in order to run the test suite, and we need the CI build to work:

Cheers.

mongoid-bot commented 6 years ago
1 Error
:no_entry_sign: One of the lines below found in CHANGELOG.md doesn’t match the expected format. Please make it look like the other lines, pay attention to periods and spaces.
1 Warning
:warning: There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.
* [#69](https://github.com/mongoid/mongoid-grid_fs/pull/69): Make Rubocop and Travis CI run again. Remove the Danger gem. - [@jonatack](https://github.com/jonatack).

Generated by :no_entry_sign: danger

jonatack commented 6 years ago

The build for PR #70 should be green once this is merged.

dblock commented 6 years ago

I can help with Danger, that works on all mongoid projects without issue, lets fix the problem and not remove it? What's the problem?

dblock commented 6 years ago

For rubocop I generally just rubocop -a ; rubocop --auto-gen-config and call it a day. I would avoid #rubocop disable unless we absolutely think it's required.

jonatack commented 6 years ago

Agreed, and that worked for me locally but it was not enough for the CI build. The auto-changes by Rubocop did not solve the issue and were a bit heavy-handed -- I prefer to make the minimum viable changes. Adding the disable comment did solve it.

I can try again without the magic comment to re-verify once the Danger gem issue is resolved.

jonatack commented 6 years ago

Hi @dblock, added the Danger gem back in and once again it appears to break the CI. Please advise?

dblock commented 6 years ago

The error is posted above. It says that one of the lines in changelog is invalid. It's nitpicking, it doesn't like the period in Remove the Danger gem. - [@jonatack](...). You don't need that line anyway, no need to mention that CI works again :) We definitely do appreciate your work though!

jonatack commented 6 years ago

@dblock when the CI was broken by Danger, I naturally looked at the build log and saw:

$ bundle exec danger
Danger has failed this build. 
Found 1 error.
The command "bundle exec danger" failed and exited with 1 during .
Your build has been stopped.

There was no mention that it could be due to the pollution that Danger is adding here. I've seen Danger respond with 3 different messages to my commits. In each case, the messages were unhelpful and confusing, when not downright wrong. This gem is causing more lost developer time than it is helping. I'm abandoning this PR and moving this gem back to passing builds and will just use my fork. Cheers.

dblock commented 6 years ago

cc: @orta ^ danger should probably say why it's failing on the command line too

@jonatack if you want to improve messages in danger-changelog, it's here

Thanks for your help @jonatack.

orta commented 6 years ago

Pretty sure if you run with --verbose it will, can't remember how the ruby version works anymore