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

Support YARD notation #724

Merged
merged 10 commits into from
Jan 24, 2020
Merged

Support YARD notation #724

merged 10 commits into from
Jan 24, 2020

Conversation

tvallois
Copy link
Contributor

Hi,

The scope of this pull request is to allow annotate_models to generate models documentation using YARD. This is the first step, i'll add more features later.

Copy link
Collaborator

@drwl drwl left a comment

Choose a reason for hiding this comment

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

First off, thanks for submitting a PR 👍

I'm not very familiar with yard so I had some questions -

@@ -904,6 +904,17 @@ def classified_sort(cols)
([id] << rest_cols << timestamps << associations).flatten.compact
end

def map_col_type_to_ruby_classes(col_type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious - can you elaborate why it should not be private and public instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, my first idea was that it can be reused somewhere else but seems not yet so i'll add it in private

@@ -957,6 +957,7 @@ def map_col_type_to_ruby_classes(col_type)
when 'datetime', 'timestamp', 'time' then Time.to_s
when 'date' then Date.to_s
when 'text', 'string', 'binary', 'inet', 'uuid' then String.to_s
when 'json', 'jsonb' then Hash.to_s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a model has a json/jsonb column, it's transformed in Hash class in ActiveRecord. That's the same logic i've followed here.

@@ -913,6 +908,7 @@ def map_col_type_to_ruby_classes(col_type)
when 'date' then Date.to_s
when 'text', 'string', 'binary', 'inet', 'uuid' then String.to_s
when 'json', 'jsonb' then Hash.to_s
when 'boolean' then 'Boolean'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this change any current behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems not. Boolean is a class that YARD understand quite well. It's better than TrueClass and FalseClass

Choose a reason for hiding this comment

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

If you are referring to just the boolean case, Boolean is the string for what YARD expects a boolean type to be in the comments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha

@drwl
Copy link
Collaborator

drwl commented Jan 22, 2020

@tvallois looks like it's good to go. If you can resolve merge conflicts I'm happy to merge this in next 🙂

@tvallois
Copy link
Contributor Author

@tvallois looks like it's good to go. If you can resolve merge conflicts I'm happy to merge this in next

done :)

@drwl drwl merged commit 0625547 into ctran:develop Jan 24, 2020
@Samsinite
Copy link

🎉 excited for this!

vfonic pushed a commit to vfonic/annotate_models that referenced this pull request May 8, 2020
The scope of this pull request is to allow annotate_models to generate models documentation using YARD. This is the first step, I'll add more features later.
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.

None yet

3 participants