Skip to content

Replace all BorderRadius.circular with const BorderRadius.all#91239

Merged
fluttergithubbot merged 7 commits into
flutter:masterfrom
grafovdenis:apply_const_border_radius
Oct 7, 2021
Merged

Replace all BorderRadius.circular with const BorderRadius.all#91239
fluttergithubbot merged 7 commits into
flutter:masterfrom
grafovdenis:apply_const_border_radius

Conversation

@grafovdenis

Copy link
Copy Markdown
Contributor

This PR contains such replacements: BorderRadius.circular(arg) -> const BorderRadius.all(Radius.circular(arg)).
I ran new Dart Code Metrics rule prefer-const-border-radius on flutter repo and left test/painting/border_radius_test.dart unchanged since it tests BorderRadius.circular behaviour.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard Bot added a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Oct 4, 2021
@google-cla google-cla Bot added the cla: yes label Oct 4, 2021
@bernaferrari

Copy link
Copy Markdown
Contributor

Isn't there a way to make BorderRadius.circular(arg) a const?

@grafovdenis

Copy link
Copy Markdown
Contributor Author

Isn't there a way to make BorderRadius.circular(arg) a const?

Nope, we have to create Radius object during construction.

https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/painting/border_radius.dart#L294

@Piinks Piinks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wow this is awesome thank you. LGTM

It makes me wonder if we should deprecate BorderRadius.circular and refer folks to the const option here.

Also might be a good prefer type lint, @a14n do you think that would be a good candidate? I think deprecating it might be better.

@fluttergithubbot fluttergithubbot merged commit fd12db0 into flutter:master Oct 7, 2021
@Hixie

Hixie commented Oct 8, 2021

Copy link
Copy Markdown
Contributor

The reason we have the BorderRadius.circular(arg) constructor is to avoid requiring that people create the much more verbose alternative. But certainly for people (such as us!) who prefer the verbosity that allows const, it would be an interesting lint.

Thanks @grafovdenis!

@a14n

a14n commented Oct 8, 2021

Copy link
Copy Markdown
Contributor

Also might be a good prefer type lint, @a14n do you think that would be a good candidate?

Sure! People would be free to enable this lint if they like verbosity :). Feel free to file an issue on linter package!

@grafovdenis

grafovdenis commented Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

@Piinks @Hixie @a14n
Thanks for the gratitude!
Me and dart-code-metrics team recently introduced such lint rule.
Feel free to use Dart Code Metrics as analyzer plugin instead of making a copy 😊

@incendial

incendial commented Oct 8, 2021

Copy link
Copy Markdown

Sure! People would be free to enable this lint if they like verbosity :). Feel free to file an issue on linter package!

I'm generally curious, do you have a limitation on using community packages? Coz we provide an analyzer plugin with this rule already implemented, but you want to implement the same rule in the linter package instead.

@incendial

Copy link
Copy Markdown

For instance, if the motivation is not to depend on any community package (coz they can be abandoned, contain malicious code, etc.), then it's totally understandable.

@Hixie

Hixie commented Oct 8, 2021

Copy link
Copy Markdown
Contributor

It's more about minimizing the number of dependencies for the core packages. There's a number of complicated implications about using packages from the core framework, for example we have to pin everything which means that anybody using the framework has to use the exact version of the dependency that the framework uses (even if there's a newer version), there's the impact on the license file size (which is already ~1MB!), and so on. You are more than welcome to submit PRs to the linter package though. Many of the lints in the dart-code-metrics package look super-valuable.

(It's not just about who wrote the package, either, for example we currently depend on vector_math, which was made by the Dart team, but we're seriously considering inlining the relevant code into the Flutter framework to avoid having that dependency. We literally have a test that checks that nobody is adding more dependencies and flags them when they get added.)

@incendial

Copy link
Copy Markdown

Thank you for the answer! Honestly, I was not expecting that you pin every package version and have a test for dependencies, but now it explains a lot.

@Hixie

Hixie commented Oct 8, 2021

Copy link
Copy Markdown
Contributor

Yeah, we started doing that because otherwise any dependency that made a slight change that stopped the tool from compiling would mean that all our users would get stuck on a broken version and the tool would be unable to run to get them fixed. (And also any dependency that made an analyzer error pop up or anything like that on any of our tests anywhere would cause our CI to go red, which was happening at least once a week before we pinned everything.)

@BirjuVachhani

BirjuVachhani commented Feb 16, 2022

Copy link
Copy Markdown

Isn't there a way to make BorderRadius.circular(arg) a const?

I can see that Radius.circular is now a const constructor then why can't BorderRadius.circular be a const constructor as well?

I think having BorderRadius.circular is very neat rather than going with BorderRadius.all to achieve the same thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants