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.

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

✨ Add package to generated assets #401

Merged
merged 12 commits into from
Oct 19, 2023
Merged

✨ Add package to generated assets #401

merged 12 commits into from
Oct 19, 2023

Conversation

AlexV525
Copy link
Member

What does this change?

Add package and the default value of package to the generated asset class and integrated assets.

Fixes #399 🎯

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
    • Ensure the tests (melos run unit:test)
    • Ensure the analyzer and formatter pass (melos run format to automatically apply formatting)
  • Appropriate docs were updated (if necessary)

@AlexV525
Copy link
Member Author

@wasabeef Could you take a first look and let me know if you have some other thoughts? I haven't write tests or docs for this yet.

@wasabeef
Copy link
Member

@AlexV525
That sounds good.

@AlexV525
Copy link
Member Author

@AlexV525 That sounds good.

Thanks. I'm going to have a few days break and I'll be back to update this then.

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #401 (ca1e1e6) into main (a0ae85f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
+ Coverage   98.26%   98.28%   +0.01%     
==========================================
  Files          20       20              
  Lines         691      698       +7     
==========================================
+ Hits          679      686       +7     
  Misses         12       12              
Files Coverage Δ
packages/core/lib/generators/assets_generator.dart 98.15% <100.00%> (+0.07%) ⬆️
...lib/generators/integrations/flare_integration.dart 100.00% <100.00%> (ø)
.../core/lib/generators/integrations/integration.dart 100.00% <100.00%> (ø)
...ib/generators/integrations/lottie_integration.dart 92.85% <100.00%> (-0.25%) ⬇️
.../lib/generators/integrations/rive_integration.dart 100.00% <100.00%> (ø)
...e/lib/generators/integrations/svg_integration.dart 100.00% <100.00%> (ø)

Comment on lines 91 to 92
@Deprecated('Do not use package for a package asset')
String? package = package,
Copy link
Member Author

Choose a reason for hiding this comment

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

The dart_style formatter will produce a different result for this line.

@AlexV525
Copy link
Member Author

AlexV525 commented Jul 5, 2023

I ran formatting tests locally which resulted in success.
image

@AlexV525
Copy link
Member Author

AlexV525 commented Jul 5, 2023

It's a disaster when using @Deprecated along with dart_style...

@AlexV525 AlexV525 marked this pull request as ready for review July 5, 2023 07:59
@AlexV525 AlexV525 requested a review from wasabeef as a code owner July 5, 2023 07:59
README.md Outdated
flutter_gen:
assets:
outputs:
package_parameter_enabled: false # <- Add this line.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: package_parameter_enabled seems quite long, surely package_parameter would also be acceptable?

and in your example, would you set this to "true" to have the package parameter included?

Copy link
Member Author

@AlexV525 AlexV525 Aug 22, 2023

Choose a reason for hiding this comment

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

nit: package_parameter_enabled seems quite long, surely package_parameter would also be acceptable?

The parameter is not new, so it'd be a breaking change if we use another name.

and in your example, would you set this to "true" to have the package parameter included?

Sure. Sounds good too.

@AlexV525
Copy link
Member Author

@bramp Can you help with merging and publishing?

@bramp
Copy link
Contributor

bramp commented Aug 22, 2023

@AlexV525 I'd be happy to review, but I'm not a maintainer of this project. I just came across this PR when I realised I needed the package name.

@AlexV525
Copy link
Member Author

cc @lcdsmao

@wasabeef wasabeef added this to the 5.4.0 milestone Sep 21, 2023
@VB10
Copy link

VB10 commented Oct 19, 2023

how is going this feature, it so useful with using modular import.

# Conflicts:
#	packages/core/lib/generators/assets_generator.dart
@AlexV525 AlexV525 merged commit 30d50aa into FlutterGen:main Oct 19, 2023
5 checks passed
@AlexV525 AlexV525 deleted the feat/add-package-to-gen branch October 19, 2023 02:26
SvgPicture svg({
Key? key,
bool matchTextDirection = false,
AssetBundle? bundle,
String? package = 'example_resources',
@Deprecated('Do not use package for a package asset')
Copy link
Contributor

@bramp bramp Oct 19, 2023

Choose a reason for hiding this comment

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

In the deprecated message, what should the developer do instead? "Do not use" doesn't tell them what to do instead.

@deprecated("Package is no longer needed as it now defaults to correct package") ?

Copy link
Member Author

Choose a reason for hiding this comment

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

They should not use package for a package asset, which literally means not using it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah and it's @deprecated actually.

@@ -84,7 +88,7 @@ class AssetGenImage {
bool matchTextDirection = false,
bool gaplessPlayback = false,
bool isAntiAlias = false,
String? package = 'example_resources',
@deprecated String? package = package,
Copy link
Contributor

Choose a reason for hiding this comment

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

No deprecate message?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because of a dart_style formatting issue and I cannot get a consist behavior when generating files with @Deprecated, so go for the @deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah sorry you did explain that back in July. Sorry for asking again!

I just tried again locally, and dart format does not change it when it's formatted it either:

    @Deprecated("short") String? package,

or

    @Deprecated("really really really really really really really really really really really long ")
    String? package,

Ok, it's such a minor thing, it's not worth debugging more. But maybe off-topic, but is the dart_style package still needed? dart format works fine without it.

Copy link
Member Author

Choose a reason for hiding this comment

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

but is the dart_style package still needed? dart format works fine without it.

So far it looks to be required because codes are generated when running runners and produced formatted files without calling dart format.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, dart_style is used during the generating of the code.

I noticed the version of dart_style listed in packages/core/pubspec.yaml is a year old, and the dart format being run in the GitHub actions seems a lot newer than that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed the version of dart_style listed in packages/core/pubspec.yaml is a year old, and the dart format being run in the GitHub actions seems a lot newer than that.

That's fine unless it released v3 then we'll need to manually bump it.

@@ -119,7 +123,7 @@ class AssetGenImage {

ImageProvider provider({
AssetBundle? bundle,
String? package = 'example_resources',
String? package = package,
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing deprecate message?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks!

AlexV525 added a commit that referenced this pull request Oct 27, 2023
## What does this change?

Further updates to #401.

## Type of change

- [x] Bug fix (non-breaking change which fixes an issue)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR]: Generate the package into the Assets/Fonts class
4 participants