-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[google_fonts] Extract the config class to its own file in prevision of a GoogleFontsLite version #11602
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
[google_fonts] Extract the config class to its own file in prevision of a GoogleFontsLite version #11602
Changes from all commits
91027e4
0a33c95
c4d3235
aaeb7d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,3 +3,4 @@ | |
| // found in the LICENSE file. | ||
|
|
||
| export 'src/google_fonts_all_parts.dart'; | ||
| export 'src/google_fonts_config.dart'; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,23 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Copyright 2013 The Flutter Authors | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Use of this source code is governed by a BSD-style license that can be | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // found in the LICENSE file. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import 'package:http/http.dart' as http; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
TheCarpetMerchant marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// A collection of properties used to specify custom behavior of the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// GoogleFonts library. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class GoogleFontsConfig { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Whether or not the GoogleFonts library can make requests to | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// [fonts.google.com](https://fonts.google.com/) to retrieve font files. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool allowRuntimeFetching = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// The HTTP client used to fetch fonts. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// If this is null, a shared default [http.Client] will be used. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// If you supply a client, you are responsible for closing it. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| http.Client? httpClient; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+9
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renaming the To maintain backward compatibility while adopting the more specific name, consider adding a deprecated typedef for
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are good points. We should try to avoid a breaking change unless absolutely necessary, and in order to allow users to access this class, it should be exported.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This just clutters the namespace for nothing, especially with such a common name as "Config". Still, if you'd rather not have a breaking change I'll export the google_fonts_config file. It really does seem like a negative to me though.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation! I agree that Config is a very generic name and renaming it to GoogleFontsConfig is a solid improvement to avoid cluttering the namespace. However, we still need to export GoogleFontsConfig because otherwise, developers cannot reference the type directly (e.g., they won't be able to write Since we recently bumped the major version, using a deprecation path is best to avoid churn on the ecosystem. We can expose a deprecated typedef in google_fonts_config.dart and export it in lib/google_fonts.dart. We can then clean up and remove this deprecation in a future major version bump. @Deprecated('Use GoogleFontsConfig instead')
typedef Config = GoogleFontsConfig;What do you think about using this deprecation path?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Deprecated('Use GoogleFontsConfig instead') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| typedef Config = GoogleFontsConfig; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+22
to
+23
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will still need a public doc comment on it to pass analysis:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This documentation is still needed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is deprecated though in this change, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.