-
Notifications
You must be signed in to change notification settings - Fork 44
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
Various improvements #41
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this and for splitting it up from the other bigger PR. Overall, there are a lot of good changes here, but I did leave comments on a few things I'd like to see changed. And of course all the test will need to be passing before it can be merged.
.ruby-version
Outdated
@@ -1 +0,0 @@ | |||
2.6.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this was deleted, but I'd prefer to keep it around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gem is supporting multiple Ruby versions. Why do we need to lock at specific? The story like for Gemfile.lock
, short answer with link to article you can find here: https://stackoverflow.com/a/4151540/2630849
Bundler.require :default, :development | ||
|
||
Combustion.initialize! :action_controller | ||
run Combustion::Application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was needed to be able to load up the combustion app for manual testing and such. Was it removed on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's needed for manual testing, yeah. But do we need for manual testing when we have good fast auto-tests? I can return it, but I'm against manual tests in gems, just let's cover the gem with 100% and add specs for any issue we will find.
c78f24d
to
e10f47b
Compare
e10f47b
to
94ca612
Compare
Rebased to |
94ca612
to
993e95e
Compare
Support of Rails 6 added. |
@adamcrown friendly reminder. |
993e95e
to
a461d93
Compare
Another reminder… |
end_of_line = lf | ||
charset = utf-8 | ||
trim_trailing_whitespace = true | ||
insert_final_newline = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not required, but it helps to configure an editor of any contributor to right (for project) settings and prevent too long lines, whitespaces at line ends, no EOL symbols in files, etc. Like this: https://github.com/biola/Voight-Kampff/pull/41/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5L54
It's pretty common standard, without OS or editor lock, so I don't see a problem with it, only help.
|
||
require 'voight_kampff/test' | ||
require 'voight_kampff/methods' | ||
require 'voight_kampff/rack_request' if defined?(Rack::Request) | ||
require 'voight_kampff/engine' if defined?(Rails::Engine) | ||
|
||
# Class helper methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try to run RuboCop without it?
require 'pathname' | ||
Pathname.new File.expand_path '..', File.dirname(__FILE__) | ||
end | ||
ROOT = File.expand_path '..', __dir__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to use brackets for more readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're writing text in natural language (like English) mostly without parentheses, so "readability" is very subjective.
module VoightKampff | ||
# Integration with Rails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary for RuboCop success.
# frozen_string_literal: true | ||
|
||
module VoightKampff | ||
# Helper for Rack::Request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
[ | ||
(Rails.root if defined? Rails), | ||
Dir.pwd, | ||
VoightKampff::ROOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constant ROOT is needed only here, may be calculate root path of the gem here without unnecessary constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made it similar to Rails.root
at least, I yes, it's using only in single place right now, but I also think this approach is more safe if the test.rb
file will be renamed or just a method will be moved to another file. I see no problem with this simple constant. Again: it's more mnemonic to Rails and maybe even Dir.pwd
, also it adds readability (without additional ..
s and File
and ()
).
Removed ------- * `rack` as dependency * Unused `config.ru` Changed ------- * Default path for cache is now in `./tmp` directory instead of `./config` Added ----- * Support of Rails 6 Development ----------- * [EditorConfig](https://editorconfig.org/) added * [RuboCop](https://github.com/rubocop-hq/rubocop) added and offenses fixed * RuboCop task added for Travis CI * Drop support for Ruby <= 2.2 (they even will not get security patches) * Ignore built gems for git
a461d93
to
48236e0
Compare
🆗 ) |
@adamcrown friendly reminder. |
There is already Rails 7… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Sorry for the very late response to this. If you can fix the conflicts, I'll be happy to merge it and push up a new version.
And resolve it's offenses. Including new metadata in gemspec.
Done. I'd glad to discuss #38 too, I see unfinished discussion there. |
Removed
rack
as dependencyconfig.ru
Changed
./tmp
directory instead of./config
Added
Development