Skip to content

Ensure formatted token is a call before forcing line breaks #1254

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

Merged
merged 5 commits into from
Feb 11, 2025

Conversation

MichaelChirico
Copy link
Contributor

Closes #1253.

Appreciate some guidance on how to sew this up into a complete PR.

@MichaelChirico
Copy link
Contributor Author

PS it might help to write something about fledge in the CONTRIBUTING guide, I am not sure how to approach ensuring there is a proper NEWS entry.

https://github.com/r-lib/styler/blob/main/CONTRIBUTING.md

Copy link
Contributor

github-actions bot commented Feb 7, 2025

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

  • ✔️cache_applying: 26.4ms -> 26.2ms [-3.84%, +2.43%]
  • ✔️cache_recording: 391ms -> 390ms [-0.75%, +0.54%]
  • ✔️without_cache: 913ms -> 911ms [-0.62%, +0.29%]

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

Copy link

codecov bot commented Feb 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.64%. Comparing base (b18a1cb) to head (e7b1565).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1254   +/-   ##
=======================================
  Coverage   91.64%   91.64%           
=======================================
  Files          46       46           
  Lines        2656     2658    +2     
=======================================
+ Hits         2434     2436    +2     
  Misses        222      222           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Feb 8, 2025

@MichaelChirico thanks for the contribution. Actually I don't use {fledge} anymore. It did not help me to make my release workflow faster since it failed multiple times when I tried it and the commands were not transparent (enough) to me. Can you please remove the reference to the PR? It's not really the convention in this repo, I mostly use git blame to find the PR if I need to. I.e. I'll add a news bullet only when I draft a release.

Copy link
Contributor

github-actions bot commented Feb 8, 2025

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

  • ✔️cache_applying: 25.8ms -> 25.9ms [-1.85%, +2.77%]
  • ✔️cache_recording: 376ms -> 377ms [-0.08%, +0.85%]
  • ✔️without_cache: 878ms -> 880ms [-0.06%, +0.65%]

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

@MichaelChirico
Copy link
Contributor Author

Can you please remove the reference to the PR?

Sorry, is this about {fledge} (which I found at the top of the NEWS when considering an entry), or the comment about the issue # in the test suite?

@lorenzwalthert
Copy link
Collaborator

Two separate things:

  • no need to worry about fledge and NEWS, I'll adapt before the in the next release. I'll remove the reference to fledge there too.
  • In the diff you introduced, please remove the references to the PR / issue number

@MichaelChirico
Copy link
Contributor Author

OK, done

Copy link
Contributor

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

  • ✔️cache_applying: 26.5ms -> 26.2ms [-3.01%, +0.96%]
  • ❗🐌cache_recording: 415ms -> 419ms [+0.22%, +1.36%]
  • ✔️without_cache: 968ms -> 969ms [-1.23%, +1.4%]

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

@lorenzwalthert
Copy link
Collaborator

Great, thanks @MichaelChirico

@lorenzwalthert lorenzwalthert merged commit 8b43fe2 into r-lib:main Feb 11, 2025
17 checks passed
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.

Strange styling suggestion
2 participants