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.

Sign up for GitHub

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

Jump to bottom

Enable groups for constants (Resolves #610) #1056

Merged
merged 4 commits into from
Apr 22, 2018
Merged

Enable groups for constants (Resolves #610) #1056

merged 4 commits into from
Apr 22, 2018

Conversation

Kjarrigan
Copy link
Contributor

@Kjarrigan Kjarrigan commented Jan 12, 2017 •
edited
Loading

Description

I started contributing to gosu this month and while working on some issues there I stumbled upon a huge list of constants in the docs and it would be really helpfull if we could group and collapse them. @jlnr found an old issue #610 which stated that this is a feature I have to implement on my own. So this is my first approach. Its definitely not finished yet, but the easy part is done and its functional but not that pretty yet. So I would like to get some feedback if my basic idea was right and if I can look further into this to make it pretty.

Screenshots

const_summary_expanded
const_summary_collapsed

ToDo

  • Change the stylesheet to make the folded list look like the folded method list (all names in colored boxes, no "linebreak" after each name)
  • Add a test

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@coveralls
Copy link

coveralls commented Jan 12, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.0007%) to 93.495% when pulling cdc8678 on Kjarrigan:const-collapse into ea6ed94 on lsegal:master.

@Kjarrigan Kjarrigan mentioned this pull request Jan 12, 2017
Merged
@coveralls
Copy link

coveralls commented Jan 12, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.001%) to 93.497% when pulling 9944dd4 on Kjarrigan:const-collapse into 915fb1f on lsegal:master.

lsegal
lsegal requested changes Jan 12, 2017
View reviewed changes
Copy link
Owner

@lsegal lsegal left a comment

Choose a reason for hiding this comment

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

This mostly looks good but seems to break tests. If those are fixed and comments are addressed I would not be opposed to merging this.

templates/default/fulldoc/html/js/app.js Outdated
else if (next.hasClass('summary')) {
var list = $('<dl class="constants summary compact" />');
list.html(next.html());
list.find('.docstring').remove();
Copy link
Owner

Choose a reason for hiding this comment

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

Why are individual elements being removed here (including elements below). Why not just use CSS to style 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.

The current implementation of the method-summaries remove some tags as well so I just copied it but I think your right.

lib/yard/code_objects/constant_object.rb Outdated
# The scope of the constants is always +:class+
#
# @return [Symbol] +:class+
def scope
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure how this method is being used. Is there a reason this was added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are no groups, but you have constants it raises an exception NoMethodError: undefined method scope'` like it did with the classvariable in the failing test altough I'm not sure why this is triggered but I'll definitely look into it.

Copy link
Owner

Choose a reason for hiding this comment

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

This probably happens because of some shared logic in the templates. You may want to change the code in the template itself rather than adding new APIs. I'm not sure a #scope method makes sense for a ConstantObject.

@Kjarrigan
Copy link
Contributor Author

Hi @lsegal, thanks for your feedback. Wasn't sure if my approach would fit into the lib as it's quite some code and I had some problems figuring out where to put my stuff. But I'll now go on and fix the tests, add a new one for the constant groups and look into your comments.

@thomthom thomthom mentioned this pull request Jan 20, 2017
Open
@Kjarrigan
Copy link
Contributor Author

blow the dust away

Sorry for the long break but I finally looked into this PR again and this time finished it. I reworked the logic, fixed the extisting tests and added a new one. So I guess @lsegal I'm ready for another review.

Attached are two screenshots with a collapsed and an expanded view. If the constants are collapsed the value is shown as tooltip as seen in the first screenshot.

PS: As far as I can tell the failing test on ruby-head is unrelated to this PR.
collapsed
expanded

@Kjarrigan
Copy link
Contributor Author

Sleeping over it I remembered that I didn't test/considered deprecated constants. They work as well but I thought that it would be cool if you can see some indication in the collapsed mode as well - so I added a bit of js and css to make it red and add the Word "Deprecated." in the tooltip as well.

See the attached image.

As far as I can tell there are no tests for the Javascript?
deprecated_const

@Kjarrigan
Copy link
Contributor Author

Rebased and everything green now.

@Kjarrigan
Copy link
Contributor Author

Is this repo still alive?

@lsegal
Copy link
Owner

lsegal commented Apr 21, 2018

Not dead, just slower, lately. I will likely make some time on this at some point this month.

@lsegal lsegal merged commit d6491b4 into lsegal:master Apr 22, 2018
@lsegal
Copy link
Owner

lsegal commented Apr 22, 2018

Thanks for the hard work on this, @Kjarrigan!

@DanRathbun
Copy link

When can we expect a new release version ?

@Kjarrigan
Copy link
Contributor Author

@lsegal Thanks for the merge!

lsegal added a commit that referenced this pull request May 28, 2018
@lsegal
Tag release v0.9.13
dff428a 
References:
  #1056, #1144, #1145, #1152, #1154, #1156, #1166, #1167,
  #1168, #1169, #14256, #610
@thomthom
Copy link
Contributor

thomthom commented Apr 8, 2019

The grouping is done by @!group and @!endgroup, right? Is this working with the C parser? (I'm not seeing any groups - but not sure if I added the tags properly in my source...

@dependabot dependabot bot mentioned this pull request Mar 12, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsegal lsegal lsegal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Kjarrigan @coveralls @lsegal @DanRathbun @thomthom