[google_fonts] Extract the config class to its own file in prevision of a GoogleFontsLite version#11602
[google_fonts] Extract the config class to its own file in prevision of a GoogleFontsLite version#11602TheCarpetMerchant wants to merge 4 commits into
Conversation
…of a GoogleFontsLite version
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration logic by moving the Config class to a new file, google_fonts_config.dart, and renaming it to GoogleFontsConfig. Feedback indicates that this rename is a breaking change and suggests adding a deprecated typedef for Config to maintain backward compatibility.
| 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; | ||
| } |
There was a problem hiding this comment.
Renaming the Config class to GoogleFontsConfig is a breaking change for any users who have explicitly typed the GoogleFonts.config object in their code.
To maintain backward compatibility while adopting the more specific name, consider adding a deprecated typedef for Config. Additionally, ensure that GoogleFontsConfig (and the typedef) is exported from the package's public API (e.g., lib/google_fonts.dart), as GoogleFonts.config is a public field and its type should be accessible to consumers.
| 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; | |
| } | |
| 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; | |
| } | |
| /// Deprecated. Use [GoogleFontsConfig] instead. | |
| @Deprecated('Use GoogleFontsConfig instead') | |
| typedef Config = GoogleFontsConfig; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This just clutters the namespace for nothing, especially with such a common name as "Config".
The only purpose of the class was to be a clear separation between configuration fields and the rest of the GoogleFonts class, there is no utility to developers having their own instance for other purposes (again, especially with this unfortunate "Config" name). As a dev I would rather not have this in my namespace and I see removing it as an improvement.
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.
There was a problem hiding this comment.
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 GoogleFontsConfig cfg = GoogleFonts.config; or pass it to configuration helper functions).
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?
| 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; | ||
| } |
There was a problem hiding this comment.
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.
|
Thank you for putting this together to make #11433 more robust! |
| @Deprecated('Use GoogleFontsConfig instead') | ||
| typedef Config = GoogleFontsConfig; |
There was a problem hiding this comment.
This will still need a public doc comment on it to pass analysis:
/// Deprecated. Use [GoogleFontsConfig] instead.
There was a problem hiding this comment.
This documentation is still needed.
I can update the changelog, but I'm unsure of what you would want changed in the pubspec ? |
|
You'll have to bump the package version in the pubspec so we can release it. |
Ah, I was thinking of waiting for the GoogleFontsLite commit to make a release. Making one for just a change in which file hosts a particular class doesn't seem very useful ? |
It does. This changes the public API. We don't batch changes for releases of this package. |
Done |
Piinks
left a comment
There was a problem hiding this comment.
Hi @TheCarpetMerchant thanks for updating the pubspec and the changelog. This isn't quite ready to merge yet. Would you like me to finish up the small bits that remain so we can get this merged and unblock the other PR? I want to respect your time and feel like we have been going back and forth over some small final things to make this ready. Let me know, happy to get this in finally.
| @@ -1,3 +1,7 @@ | |||
| ## 8.2.0 | |||
|
|
|||
| - Extract the class `Config` to its own file and rename it `GoogleFontsConfig`. The `Config` name will be deprecated in a future release. | |||
There was a problem hiding this comment.
It is deprecated though in this change, isn't it?
There was a problem hiding this comment.
| - Extract the class `Config` to its own file and rename it `GoogleFontsConfig`. The `Config` name will be deprecated in a future release. | |
| - Extract the class `Config` to its own file and rename it `GoogleFontsConfig`. The `Config` class is now deprecated. |
| @Deprecated('Use GoogleFontsConfig instead') | ||
| typedef Config = GoogleFontsConfig; |
There was a problem hiding this comment.
This documentation is still needed.
Sure go ahead. I'll take a look to know what to do next time. |
|
It looks like I do not have permissions to push to the PR, so I am going to close it to re-open as a fresh PR and get this in to unblock the other change: #12202 |
See #11433 and the comment #11433 (review)