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

[#1400] Group global constants and functions by file name #1401

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thePsguy
Copy link

@thePsguy thePsguy commented Sep 19, 2024

  • Make doc_index iterable.
  • After custom_categories are processed, group any remaining global constants / functions further based on the file they're in.

Staged sample: https://serve-dot-zipline.appspot.com/asset/55d078ae-b243-597e-9a9d-257f931f2723/zpc/63fc25cv8le/GMSGeometryUtils%2BFunctions.html

Resolves #1400

@@ -69,7 +69,42 @@ def self.group_custom_categories(docs, doc_index)
children.each.with_index { |child, i| child.nav_order = i }
make_group(children, category['name'], '')
end
[group.compact, docs]

globals_grouped_by_file = self.group_globals_by_file(docs, doc_index)
Copy link
Author

Choose a reason for hiding this comment

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

Hi @johnfairh, should I place this under a new command line argument or something?

You recommended including a glob/wildcard/regexp, could you please elaborate a little more on what such a regexp would be used to filtered? Would it be used to determine whether constants in a file should be affected by this logic or not?

I can make that change in a following commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The 'glob' comment was in response to your example of "children": [file:GlobalUtils.h" which is not the feature you ended up going for.

The default behaviour mustn't change because of existing users, new behaviour can be behind a CLI option.

[group.compact, docs]

globals_grouped_by_file = self.group_globals_by_file(docs, doc_index)
[group.compact, docs + globals_grouped_by_file]
Copy link
Author

Choose a reason for hiding this comment

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

Adding globals_grouped_by_file to the end of docs makes the Constants and Functions sections show up at the end of other default sections, and also prevents the other default groups from being prepended with "Other".

The order of the sections, and file level groups in them appears to be consistent to be. Is there a specific way to ensure order is maintained that you would recommend?
I could sort the files in each group, and symbols in each file alphabetically to be sure. Should I?

cc: @johnfairh

Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to go back to group_docs() and figure out the code there -- the advice to start in this routine was based (again :-)) on your other design.

Ruby hash iteration order is guaranteed - I think that is all you need to prove that you don't need any further sorting.

@@ -69,7 +69,42 @@ def self.group_custom_categories(docs, doc_index)
children.each.with_index { |child, i| child.nav_order = i }
make_group(children, category['name'], '')
end
[group.compact, docs]

globals_grouped_by_file = self.group_globals_by_file(docs, doc_index)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The 'glob' comment was in response to your example of "children": [file:GlobalUtils.h" which is not the feature you ended up going for.

The default behaviour mustn't change because of existing users, new behaviour can be behind a CLI option.

types_to_files_to_symbols = {}
# Group symbols by file and type.
doc_index.each do |index_item|
if index_item.parent_in_code.nil? && (index_item.type.name == "Constant" || index_item.type.name == "Function")
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can just use docs instead of doc_index if you just want top-level items, then won't need the parent_in_code check and will be more efficient.

[group.compact, docs]

globals_grouped_by_file = self.group_globals_by_file(docs, doc_index)
[group.compact, docs + globals_grouped_by_file]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to go back to group_docs() and figure out the code there -- the advice to start in this routine was based (again :-)) on your other design.

Ruby hash iteration order is guaranteed - I think that is all you need to prove that you don't need any further sorting.

types_to_files_to_symbols.each do |type_name, files|
types_to_file_groups[type_name] ||= []
files.each do |file, declarations|
file_group = make_group(declarations, file, '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this group is the one that should have the abstract "The following thingies are available globally" (you could write "are provided by " given that you're adding source filenames into the UI.

And the group below (line 103) is the one that should have the abstract "The following files provide thingies" or something?

@johnfairh
Copy link
Collaborator

johnfairh commented Sep 22, 2024

This is a bit different to what I thought you were proposing in #1440 with your "children": ["file:GlobalUtils.h", "regex:*CommonPattern*"] example. The big difference is that there, you were just adding stuff to existing groups. Here you're trying to automatically fabricate a hierarchy of groups, and so have the additional problem of merging those into the overall group tree: it's a more ambitious problem!

The limitations of the left nav (only showing two levels) are not great here -- I'm not sure it's a step forward to not actually see (be able to search through) the list of constants (however long). (maybe if you can show a demo I'd change my mind here)

I feel that restricting the feature to just certain kinds of declaration (here, constants and functions) is not flexible enough for general use. It would be OK to parameterise it I suppose, but I'm not sure why users would want to see a mixture of by-kind ("Other Classes/classes") and by-file ("Constants/Foo.swift/FooConstants").

Options are (1) Group all left-over declarations by kind+source-file instead of by kind; or (2) Group a configured set of kinds of left-over declarations by kind+source-file, and group other kinds by kind alone.

Both/either option needs a config option to modify the default behaviour. Option (1) is a subset of (2), for (2) should probably have an 'all kinds' shortcut.

I don't understand what the demo is showing - the constants/functions pages just look empty to me?
Screenshot 2024-09-22 at 11 49 34

Code changes look in the right kind of direction - you clearly have a good grasp of how everything fits together here.

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.

Group global constants and functions by the file they're in?
2 participants