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

Make methods private in AnnotateRoutes #598

Merged

Conversation

nard-tech
Copy link
Collaborator

@nard-tech nard-tech commented Feb 2, 2019

First I tried to extend AnnotateModels, but I found several problems in AnnotateModels and AnnotateRoutes.

In AnnotateRoutes, there were many public methods that is not used in other classes or modules.

Before my extension, I made these methods private and sort them in order of appearance.

All tests were passed.

Please trace each commits carefully. I didn't nothing special. The commits consist of coordinating code.

Note

As Code Climate shows, this gem is hard to maintain.

So I'd like to refactor this gem in the near future.

end
out << '' if magic_comments_map.any?

out += ["# #{options[:wrapper_open]}"] if options[:wrapper_open]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know you didn't make this change, but out of curiosity do you know if there's a reason behind using << versus += [] for appending to an array?

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense but this is mutating an array I believe. Line 43 is out = []

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just tested it in Pry looks like the same applies regardless of if it's a String or an Array. Functionally it looks like one can be substituted over the other so would it makes sense to change the cases so they're the same?

@drwl
Copy link
Collaborator

drwl commented Feb 19, 2019

👍 Changes look good to me. Looks a lot cleaner now that class methods are together in the singleton class rather than defined as self._.

@drwl
Copy link
Collaborator

drwl commented Feb 26, 2019

@ctran unsure if you need anything else but it should be good to merge in as it's not changing any functionality.

@drwl
Copy link
Collaborator

drwl commented Apr 27, 2019

Planning on landing this in the next few days and bumping the gem version to 3.0.0 to follow semantic versioning. Any thoughts @nard-tech or @ctran?

@drwl drwl merged commit 289621b into ctran:develop Apr 28, 2019
@drwl drwl added this to the v3.0.0 milestone Jun 16, 2019
@nard-tech nard-tech deleted the feature/make_methods_private_in_annotate_routes branch January 3, 2020 18:07
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.

3 participants