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

Refactor lib/annotate.rb #707

Merged
merged 13 commits into from
Dec 30, 2019
Merged

Refactor lib/annotate.rb #707

merged 13 commits into from
Dec 30, 2019

Conversation

drwl
Copy link
Collaborator

@drwl drwl commented Dec 25, 2019

lib/annotate.rb is bloated. This PR refactors methods out of it into Annotate::Helpers and adds test coverage for the methods.

@drwl drwl added the wip label Dec 25, 2019
@drwl drwl changed the title Refactor Refactor lib/annotate.rb Dec 25, 2019
@drwl drwl requested a review from ctran December 25, 2019 19:39
@drwl drwl added refactor and removed wip labels Dec 25, 2019
@@ -11,46 +11,46 @@ task annotate_models: :environment do
require "#{annotate_lib}/annotate/active_record_patch"

options = {is_rake: true}
ENV['position'] = options[:position] = Annotate.fallback(ENV['position'], 'before')
ENV['position'] = options[:position] = Annotate::Helpers.fallback(ENV['position'], 'before')
options[:additional_file_patterns] = ENV['additional_file_patterns'] ? ENV['additional_file_patterns'].split(',') : []
Copy link
Owner

Choose a reason for hiding this comment

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

Can this be mixed in instead of specifying full path? It would also cut back the number of changes needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ctran this is done intentionally for now to make it easier to see where everything is coming from. Do you think it would be okay to keep for now?

I'm thinking we could untangle lib/annotate.rb. I'm looking at the Pry gem as a source of inspiration.

Copy link
Owner

@ctran ctran Dec 30, 2019

Choose a reason for hiding this comment

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

I'm fine either way, was just wondering if you had specific reasons to do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to make it more explicit where it's coming from right now. I'm expecting Helpers to be a temporary solution until it's clearer what the different pieces are.

This change converts .all_options into a constant and moves it into Annotate::Constants. It also changes usages of .all_options.
@drwl drwl merged commit 3f0b6b3 into develop Dec 30, 2019
@drwl drwl deleted the drwl/work branch December 30, 2019 03:36
vfonic pushed a commit to vfonic/annotate_models that referenced this pull request May 8, 2020
This change converts .all_options into a constant and moves it into Annotate::Constants. It also changes usages of .all_options.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants