-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add UuidManagement and Callable #1
Add UuidManagement and Callable #1
Conversation
Plus some manual removals
This originally used `present?` instead of `&.empty?`
find spec -name '*_spec.rb' -exec bundle exec rspec {} \;
# include TrackBallast::UuidManagement | ||
# end | ||
# | ||
module UuidManagement |
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.
What this is expected to look like on https://www.rubydoc.info/
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.
lgtm
require "track_ballast/version" | ||
|
||
module TrackBallast | ||
class Error < StandardError; end |
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.
Would be nice to start with a logger as well.
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 think you meant to use the lib/track_ballast/error.rb
class?
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 think you meant to use the
lib/track_ballast/error.rb
class?
You're right, I did! Will follow up on that
# This will short-circuit to be `@logger` on future runs | ||
@logger = Rails.logger | ||
else | ||
raise NoLoggerError, "TrackBallast.logger is not configured" |
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.
Consider making this logger core to the TrackBallast
module instead of raising 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.
Ah, this is perhaps something that could stand to be clarified in the README.
This method will raise
because the TrackBallast.logger
method could not locate a suitable logger to use. Typically, it will default to Rails.logger
, but it could also be whatever the client of this gem decides is best. (For example, the client application could choose to route all TrackBallast
logs elsewhere.) My thinking is that it's not appropriate to completely set up a logger within this library because we don't know the circumstances in which the code is run (besides running tests, of course).
Hopefully I'm understanding your comments correctly. 😄 If I'm not, let's circle up on DMs to discuss.
Starting
TrackBallast
with two commonly used modules:UuidManagement
andCallable
The intention is to start using these versions internally. All commits have been crafted with the intention of being made public, open source code.