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

Add more vertical spacing to KLib dumps #225

Open
wants to merge 6 commits into
base: 197-consider-decl-kind-when-sorting-dump
Choose a base branch
from

Conversation

fzhinkin
Copy link
Collaborator

@fzhinkin fzhinkin commented May 6, 2024

Additional newlines should improve dump's readability and also improve diffs generated by the GH.

This change builds on #197, which groups declarations based on their kind.

Now, an extra newline will always be inserted before:

  • // Targets: meta-header,
  • a new class/interface/object/enum-class declaration
  • in-between two declarations having different kinds (for example, between a group of vars and funs).

Closes #196

@fzhinkin fzhinkin linked an issue May 6, 2024 that may be closed by this pull request
final const val examples.classes/con // examples.classes/con|{}con[0]
final fun <get-con>(): kotlin/String // examples.classes/con.<get-con>|<get-con>(){}[0]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps, we should not separate const vals, vas, and vars with a newline. Looking forward to your thoughts on that matter.

Copy link
Member

Choose a reason for hiding this comment

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

Are they sorted as different groups, e.g. consts first, vals second, vars third, or all properties alphabetically? If the latter, then we shouldn't separate them.

Copy link
Member

@ilya-g ilya-g May 22, 2024

Choose a reason for hiding this comment

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

Maybe you can show how the changes together from #224 and #225 would affect test data? I see it is already on top of #224

And make this example more complex by adding several declarations of the same type, so that the sorting rules are clear from the dump.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are they sorted as different groups, e.g. consts first, vals second, vars third, or all properties alphabetically? If the latter, then we shouldn't separate them.

They are sorted as different groups. Seeing them separated still looks a bit awkward, but maybe it's just because example have only a few declarations within each of these groups.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ilya-g I extended this test case and also added a new one, showing how grouping/sorting works with classifiers declared only for some targets.

@fzhinkin fzhinkin requested a review from ilya-g May 6, 2024 09:49
@fzhinkin fzhinkin marked this pull request as ready for review May 6, 2024 09:49
@fzhinkin fzhinkin force-pushed the 197-consider-decl-kind-when-sorting-dump branch from a9a5b5a to 41770eb Compare May 27, 2024 08:30
@fzhinkin fzhinkin force-pushed the 196-add-more-vertical-spacing branch from c1536f7 to c07e901 Compare May 27, 2024 08:30
@fzhinkin fzhinkin force-pushed the 196-add-more-vertical-spacing branch from fa9bbe7 to e8b4f6c Compare May 27, 2024 14:36
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.

Klib .api files could use some vertical spacing
2 participants