export 'package:github/src/const/language_color.dart'; #232
Conversation
export 'package:github/src/const/language_color.dart'; run language_color_generator.dart
export 'package:github/src/const/language_color.dart'; run language_color_generator.dart
|
@robrbecker can you review please ? |
lib/src/const/language_color.dart
Outdated
|
|
||
| const languagesColor = <String, String>{ | ||
| const kGitHubLanguageColors = <String, String>{ |
There was a problem hiding this comment.
Why the rename here? This name feels a bit Java. Would you be ok with just languageColors ?
| const kGitHubLanguageColors = <String, String>{ | |
| const languageColors = <String, String>{ |
There was a problem hiding this comment.
Hello Rob thanks a lot for the review!
I used this name to comply with the Flutter naming conventions. (See here).
Here is the more general naming convention for Dart.
I also prefixed it with GitHub to make it clearer.
So what do you prefer languagesColor or gitHubLanguageColors or githubLanguageColors ?
There was a problem hiding this comment.
I'm more familiar with the Dart style guide and hadn't seen the Flutter style guide that recommends the leading k. In the Dart style guide it recommends not including prefix letters https://dart.dev/guides/language/effective-dart/style#dont-use-prefix-letters and I'd prefer that approach since it keeps naming consistent with the rest of this library.
I prefer languageColors since I feel that adding github is redundant in a library named github, especially if you use a named import like
import 'package:github/github.dart' as github;
// ...
print(github.languageColors['JavaScript']);
There was a problem hiding this comment.
Also note the change of location of the s from
languagesColor to
languageColors
| @@ -46,4 +46,5 @@ export 'package:github/src/common/util/oauth2.dart'; | |||
| export 'package:github/src/common/util/pagination.dart'; | |||
| export 'package:github/src/common/util/service.dart'; | |||
| export 'package:github/src/common/util/utils.dart'; | |||
| export 'package:github/src/const/language_color.dart'; | |||
There was a problem hiding this comment.
Just a note, but now that this file is being exported, the language color map is now considered part of the public API and should be non-breaking in future changes.
tool/language_color_generator.dart
Outdated
| final stringBuffer = StringBuffer() | ||
| ..writeln('// GENERATED CODE - DO NOT MODIFY BY HAND') | ||
| ..writeln('// VERSION OF ${DateTime.now().toIso8601String()}') | ||
| ..writeln() | ||
| ..writeln('const languagesColor = <String, String>{'); | ||
| ..writeln('const kGitHubLanguageColors = <String, String>{'); |
There was a problem hiding this comment.
let's keep the name the same as above whatever we end up deciding
| ..writeln('const kGitHubLanguageColors = <String, String>{'); | |
| ..writeln('const languageColors = <String, String>{'); |
run language_color_generator.dart
|
released in 7.0.3 |
minor improvements
rename languagesColor to kGitHubLanguageColors
export 'package:github/src/const/language_color.dart';
run language_color_generator.dart