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 pluralization + clickable hyperlink support #1186

Closed
wants to merge 2 commits into from

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Mar 27, 2024

Addresses part of #1116.

For speed / usefulness, maybe should only hyperlink the files that changed?
Support pluralization + clickable hyperlink

Demo (since #1187 is merged):

image

styler 1.10.2
image

@olivroy olivroy mentioned this pull request Mar 27, 2024
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 634dce1 is merged into main:

  • ❗🐌cache_applying: 153ms -> 314ms [+101.6%, +108.54%]
  • ❗🐌cache_recording: 522ms -> 687ms [+30.67%, +32.74%]
  • ❗🐌without_cache: 997ms -> 1.16s [+15.59%, +17.15%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@olivroy olivroy changed the title Add pluralization + clickable hyperlink support NOT MERGE: Add pluralization + clickable hyperlink support Mar 27, 2024
@lorenzwalthert
Copy link
Collaborator

I don’t get why this change would slow down the touchstone benchmark so much. One reason may be that you had an old version of the base branch. Compared to the previous release, after merging your previous PR, there is no sig officiant slow down.

#868 (comment)

so I merged the main branch into your branch now. Let’s see if the issue goes away.

@olivroy
Copy link
Contributor Author

olivroy commented Mar 28, 2024

Will see. thanks for checking. Possibly that the repeated cli::format_Inline() calls slow things down. That's why I propose that maybe only do styling for files that changed?

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 208d9c7 is merged into main:

  • ❗🐌cache_applying: 155ms -> 312ms [+96.42%, +106.7%]
  • ❗🐌cache_recording: 520ms -> 688ms [+30.71%, +33.94%]
  • ❗🐌without_cache: 997ms -> 1.16s [+15.4%, +17.6%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@olivroy olivroy marked this pull request as draft April 2, 2024 12:16
@olivroy olivroy changed the title NOT MERGE: Add pluralization + clickable hyperlink support Add pluralization + clickable hyperlink support Apr 2, 2024
@olivroy
Copy link
Contributor Author

olivroy commented Apr 3, 2024

Don't really know how to make sure speed + width is preserved. Closing

@olivroy olivroy closed this Apr 3, 2024
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.

2 participants