Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Change generate-class-name for fonts, colors, assets #280

Closed
wants to merge 0 commits into from
Closed

Change generate-class-name for fonts, colors, assets #280

wants to merge 0 commits into from

Conversation

Cu-Toof
Copy link
Contributor

@Cu-Toof Cu-Toof commented Aug 15, 2022 •
edited
Loading

Change the class name that will be generated.
Update pubspec.yaml file:

flutter_gen:
  output: lib/gen/

  assets:
    outputs:
      class_name: AppAssets //default is Assets

  fonts:
    outputs:
      class_name: AppFontFamily //default is FontFamily

  colors:
    inputs:
      - assets/colors/colors.xml 
    outputs:
      class_name: AppColor //default is ColorName

@Cu-Toof Cu-Toof force-pushed the feature/change-gen-class-name-for-fonts-colors-assets branch from c2b4c6c to 0e66987 Compare August 15, 2022 14:48
thangnc
thangnc reviewed Aug 17, 2022
View reviewed changes
packages/core/lib/generators/colors_generator.dart Outdated
throw const InvalidSettingsException(
'The value of "flutter_gen/colors:" is incorrect.');
}

final buffer = StringBuffer();
final className = genColors.outputs?.className ?? 'FontFamily';
Copy link

Choose a reason for hiding this comment

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

Why did you use FontFamily in color generator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, I have fixed it.

thangnc
thangnc reviewed Aug 17, 2022
View reviewed changes
Copy link

@thangnc thangnc left a comment

Choose a reason for hiding this comment

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

Btw, you need to fix unit test @Cu-Toof

packages/core/lib/flutter_generator.dart Outdated
pubspecFile,
formatter,
flutterGen.colors,
);
Copy link

Choose a reason for hiding this comment

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

I dont think you need to update this call formatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, I have updated this code.

@thangnc
Copy link

thangnc commented Aug 17, 2022

It seems to be duplicated with this PR #279

@Cu-Toof Cu-Toof force-pushed the feature/change-gen-class-name-for-fonts-colors-assets branch 2 times, most recently from 8c66ee6 to 523b494 Compare August 18, 2022 14:02
@wasabeef wasabeef requested a review from thangnc August 21, 2022 05:44
@Cu-Toof
Copy link
Contributor Author

Cu-Toof commented Aug 21, 2022 •
edited
Loading

@thangnc I have reviewed this PR #279.
I think we can custom more than one output's information. But this pr is implementing output_class is just string type.
Replace it, I implement output as an object, code is scalable with other output's customize like file name, prefix name...

thangnc
thangnc reviewed Aug 21, 2022
View reviewed changes
Copy link

@thangnc thangnc left a comment

Choose a reason for hiding this comment

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

@Cu-Toof please fix my PR's comments. Btw, you did not add unit test?

README.md Outdated
@@ -739,16 +739,25 @@ flutter_gen:
# - snake-case
# - dot-delimiter
style: dot-delimiter
# Optional
outputs:
class_name: Assets
Copy link

Choose a reason for hiding this comment

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

I think you should add the sample class name as MyAssets and add the comment with default value is Assets

README.md Outdated

fonts:
# Optional
enabled: true
# Optional
outputs:
class_name: FontFamily
Copy link

Choose a reason for hiding this comment

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

Same as above comment. FontFamily is the default generated class name. You should add the sample class name to make it clear

README.md Outdated

colors:
# Optional
enabled: true
# Optional
inputs: []
# Optional
outputs:
class_name: ColorName
Copy link

Choose a reason for hiding this comment

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

Same as above comment. ColorName is the default generated class name

packages/core/lib/generators/assets_generator.dart Outdated
}

String _dotDelimiterStyleAssetsClassDefinition(List<_Statement> statements) {
String _dotDelimiterStyleAssetsClassDefinition(
String? className, List<_Statement> statements) {
Copy link

Choose a reason for hiding this comment

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

Please follow the current code convention. Please refer https://github.com/FlutterGen/flutter_gen/blob/main/packages/core/lib/generators/assets_generator.dart#L289

packages/core/lib/generators/assets_generator.dart Outdated
}

String _flatStyleAssetsClassDefinition(List<_Statement> statements) {
String _flatStyleAssetsClassDefinition(
String? className, List<_Statement> statements) {
Copy link

Choose a reason for hiding this comment

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

Please follow the current code convention. Please refer https://github.com/FlutterGen/flutter_gen/blob/main/packages/core/lib/generators/assets_generator.dart#L289

packages/core/lib/settings/config.dart Outdated
@@ -47,13 +47,19 @@ flutter_gen:
enabled: true
package_parameter_enabled: false
style: dot-delimiter
outputs:
class_name: Assets
Copy link

Choose a reason for hiding this comment

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

Becoz you checked in this line in Asset, so the default config no need outputs.class_name param, right?

packages/core/lib/settings/config.dart Outdated

colors:
enabled: true
inputs: []
outputs:
class_name: ColorName
Copy link

Choose a reason for hiding this comment

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

Same above comment becoz you checked in this line https://github.com/FlutterGen/flutter_gen/pull/280/files#diff-4f1088f5086a316577b245788f11453e91629f92b823d1fa58936750f6f1f518R26

packages/core/lib/settings/config.dart Outdated

fonts:
enabled: true
outputs:
class_name: FontFamily
Copy link

Choose a reason for hiding this comment

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

Same as above comment becoz you checked here https://github.com/FlutterGen/flutter_gen/pull/280/files#diff-74b9255cc2b20267c715b8162a5fcd4e4d55b3b0fb333a5bc23b52deb5d207e4R22

@wasabeef wasabeef added this to the 4.4.0 milestone Aug 21, 2022
@wasabeef
Copy link
Member

I will merge this PR after all fix.

@thangnc I appreciate your help.

@Cu-Toof Cu-Toof force-pushed the feature/change-gen-class-name-for-fonts-colors-assets branch from 523b494 to 055771f Compare September 2, 2022 01:48
@Cu-Toof Cu-Toof closed this Sep 2, 2022
@Cu-Toof Cu-Toof force-pushed the feature/change-gen-class-name-for-fonts-colors-assets branch from 055771f to 2fa5eb0 Compare September 2, 2022 01:53
@Cu-Toof Cu-Toof mentioned this pull request Sep 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thangnc thangnc thangnc left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Cu-Toof @thangnc @wasabeef