styler icon indicating copy to clipboard operation
styler copied to clipboard

Default to styling all supported file formats

Open IndrajeetPatil opened this issue 3 years ago • 13 comments
trafficstars

Closes #963

IndrajeetPatil avatar Jul 20 '22 09:07 IndrajeetPatil

@lorenzwalthert All green now.

IndrajeetPatil avatar Jul 20 '22 09:07 IndrajeetPatil

Thaks for the PR 😄. A few comments:

  • the API file should also be updated. Can you install r-lib/pkgapi and re-run roxygenize?
  • If it is a breaking change (which I am not sure yet), we should probably not merge it into main (but like rc-2.0.0), fix all the other issues in #769 and then release v2.0.0.

lorenzwalthert avatar Jul 20 '22 09:07 lorenzwalthert

The API file is already updated. precommit caught that.

IndrajeetPatil avatar Jul 20 '22 10:07 IndrajeetPatil

If it is a breaking change (which I am not sure yet)

Okay, let me know when you come to a decision on this point. It also doesn't feel like a breaking change to me, bur rather a major change.

IndrajeetPatil avatar Jul 20 '22 10:07 IndrajeetPatil

Hmm, why did the Continuous Benchmarks GHA fail?

IndrajeetPatil avatar Jul 21 '22 13:07 IndrajeetPatil

Why did {touchstone} fail?

@assignUser can you help here? I think it's related to some non-R code that you contributed that I can't understand. Same problems occured in https://github.com/lorenzwalthert/touchstone/issues/97#issuecomment-1162140464.

lorenzwalthert avatar Jul 21 '22 17:07 lorenzwalthert

The Commenting step failed because the main touchstone workflow failed and this correctly added a ❌ to the check suite. The actual touchstone run timed out during dependency installation, so probably an issue unrelated to the PR/touchstone.

So overall everything works as intended (except the time out 😁 ), I would try re-running/re-triggering the benchmark

assignUser avatar Jul 21 '22 18:07 assignUser

@assignUser oh yes right. Thanks for explaining. So basically every time the first part of the action fails, this same error ocurrs, right? Guess that's just something we have to know if errors occur and then dig deeper... Anyways, I restarted the benchmark. It has an old rspm checkpoint so I am surprised it failed. Let's see what happens...

lorenzwalthert avatar Jul 21 '22 19:07 lorenzwalthert

Codecov Report

Merging #965 (dc3c099) into main (a3b69e4) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #965   +/-   ##
=======================================
  Coverage   90.07%   90.07%           
=======================================
  Files          47       47           
  Lines        2660     2660           
=======================================
  Hits         2396     2396           
  Misses        264      264           
Impacted Files Coverage Δ
R/ui-styling.R 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov-commenter avatar Jul 26 '22 13:07 codecov-commenter

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

  •   :ballot_box_with_check:cache_applying: 26.1ms -> 25.9ms [-1.53%, +0.11%]
  •   :ballot_box_with_check:cache_recording: 1.24s -> 1.24s [-0.65%, +1.19%]
  •   :ballot_box_with_check:without_cache: 3.38s -> 3.38s [-0.58%, +0.64%]

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

github-actions[bot] avatar Aug 06 '22 18:08 github-actions[bot]

GitHub Actions now passing, had to update some config, see #969.

lorenzwalthert avatar Aug 06 '22 20:08 lorenzwalthert

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

  •   :ballot_box_with_check:cache_applying: 33.6ms -> 33.4ms [-3.18%, +2.02%]
  •   :ballot_box_with_check:cache_recording: 1.4s -> 1.39s [-2.18%, +1.57%]
  •   :ballot_box_with_check:without_cache: 3.73s -> 3.76s [-0.8%, +2.04%]

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

github-actions[bot] avatar Aug 06 '22 21:08 github-actions[bot]

Thanks, @lorenzwalthert! Yes, the builds are indeed passing now.

Btw, having slept a bit more on it, I don't think this would qualify as a breaking change. A breaking change would have been if we had stopped supporting styling a file type. Here, we continue to support everything we were supporting and some more file types.

IndrajeetPatil avatar Aug 07 '22 05:08 IndrajeetPatil

yeah, you are right. I marked it as user-facing change.

lorenzwalthert avatar Aug 15 '22 06:08 lorenzwalthert

Thanks 🎉

lorenzwalthert avatar Aug 15 '22 06:08 lorenzwalthert

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

  •   :ballot_box_with_check:cache_applying: 27.8ms -> 28ms [-0.3%, +1.44%]
  •   :ballot_box_with_check:cache_recording: 1.13s -> 1.13s [-1.11%, +0.1%]
  •   :ballot_box_with_check:without_cache: 3.01s -> 3s [-0.63%, +0.57%]

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

github-actions[bot] avatar Aug 15 '22 06:08 github-actions[bot]