fix: multiple generations for same class #123

Rodsevich · 2024-12-06T05:14:54Z

Now multiple @envied annotations generate multiples classes

Fixes Multiple generations for same class #122
It's ready for publish with 1.0.1 version

techouse · 2024-12-10T07:41:31Z

.gitignore

@@ -1,7 +1,7 @@
 .idea
 .vscode
 melos_envied.iml
-pubspec_overrides.yaml
+# pubspec_overrides.yaml


Please remove this, as the overrides should not be tracked.

techouse · 2024-12-10T07:44:36Z

examples/envied_example/pubspec.yaml

-dependencies:
-  envied: ^1.0.0
+  build_runner: ^2.4.13
+  envied_generator: 


I'm not sure why this is necessary. This is an example and should not really be run from the package itself. If desired, one can still create a pubspec_overrides.yaml file.

techouse · 2024-12-10T07:45:15Z

examples/envied_example/pubspec.yaml

+  envied_generator: 
+    path: ../../packages/envied_generator
+
+dependency_overrides: 


This should be solved with pubspec_overrides.yaml

techouse · 2024-12-10T07:45:37Z

melos.yaml

@@ -3,10 +3,6 @@ name: envied
 packages:
  - "**"

-command:
-  bootstrap:
-    usePubspecOverrides: true


I found this error in my IDE (check the melos VSCode extension):

But thats bc it was deprecated in version 3.0.0 in this issue

techouse · 2024-12-10T07:46:15Z

packages/envied_generator/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 1.0.1


Please do NOT create a new version unless you are the package maintainer.

techouse · 2024-12-10T07:47:19Z

packages/envied_generator/pubspec.yaml

@@ -1,6 +1,6 @@
 name: envied_generator
 description: Generator for the Envied package. See https://pub.dev/packages/envied.
-version: 1.0.0
+version: 1.0.1


Please do NOT create a new version unless you are the package maintainer.

techouse · 2024-12-10T07:48:58Z

packages/envied_generator/pubspec_overrides.yaml

@@ -0,0 +1,3 @@
+dependency_overrides:


This file should not be included.

techouse · 2024-12-10T07:55:40Z

Hey, I left some comments.

Please do NOT update the version in this PR.

Rodsevich · 2024-12-10T13:06:08Z

With regards to the rest of the comments:

pubspec_overrides are necessary for a correct local development. For us, that we don't have them, in order to run them we have to lose time fixing dependencies before being productive. I don't find why to don't commit them.
As a package maintainer myself, I appreciate more the guys that help me with the chores of a package maintenance. Several times I delayed the publish because of lack of time to doing that shit myself (instead of just running git pull && pub publish). If you don't like that, you can kindly thank the extra job done and say not to worry afterwards to the guys that loose their time trying to help.
With that said, and due to

Please do NOT update the version in this PR.

I'm not sure what to do... Do I remove the pubspec version upgrade and changelog (update 'em) or left them like they are in this PR?
With regards to the pubspec_overrides, I think they are better present that absent, and like I left them the project is just working. Why don't you remoe mines but add yours and leave the project functional for future contributors?

techouse · 2024-12-10T13:19:05Z

pubspec_overrides are necessary for a correct local development. For us, that we don't have them, in order to run them we have to lose time fixing dependencies before being productive. I don't find why to don't commit them.

Correct, for local development. Melos creates them anyway when bootstrapping the project.

As a package maintainer myself, I appreciate more the guys that help me with the chores of a package maintenance. Several times I delayed the publish because of lack of time to doing that shit myself (instead of just running git pull && pub publish). If you don't like that, you can kindly thank the extra job done and say not to worry afterwards to the guys that loose their time trying to help.

Don't take this the wrong way, I'm very grateful for any contribution, however, this PR is supposed to add a feature, not create a new release. The release itself is done via a Github action.

I'm not sure what to do... Do I remove the pubspec version upgrade and changelog (update 'em) or left them like they are in this PR?

Correct, remove any changes to the pubspec.yaml and changelog because it interferes with the release process. I will do that in a release PR. There is another PR waiting to be reviewed / merged which could go into a hypothetical v1.0.1.

With regards to the pubspec_overrides, I think they are better present that absent, and like I left them the project is just working. Why don't you remoe mines but add yours and leave the project functional for future contributors?

I disagree. It creates unnecessary clutter.

This reverts commit 22a2820.

techouse · 2024-12-10T16:21:39Z

packages/envied_generator/lib/src/generator.dart

+    final enviedAnnotations = element.metadata
+        .where((a) => a.element?.displayName == 'Envied')
+        .map((e) => ConstantReader(e.computeConstantValue()));
+    var generatedClassesAltogether = StringBuffer();


Please make this final instead of using a var and declare it with an explicit type.

techouse · 2024-12-10T16:22:04Z

packages/envied_generator/lib/src/generator.dart

@@ -31,6 +31,23 @@ final class EnviedGenerator extends GeneratorForAnnotation<Envied> {
    ConstantReader annotation,
    BuildStep buildStep,
  ) async {
+    final enviedAnnotations = element.metadata


Can you please add explicit type definitions?

techouse · 2024-12-10T16:23:12Z

packages/envied_generator/lib/src/generator.dart

+        .where((a) => a.element?.displayName == 'Envied')
+        .map((e) => ConstantReader(e.computeConstantValue()));
+    var generatedClassesAltogether = StringBuffer();
+    for (final a in enviedAnnotations) {


Can you please more descriptive variable names and also explicitly type them as it makes code easier to review.

Rodsevich · 2024-12-10T18:14:18Z

Sorry, dude. I think we are not compatible in our work of working and I'm not willing to continue with this. I push the changes I have already done and when I have the time I'll continue with my own implementation of this package

techouse · 2024-12-10T18:21:38Z

Sorry, dude. I think we are not compatible in our work of working and I'm not willing to continue with this. I push the changes I have already done and when I have the time I'll continue with my own implementation of this package

Yeah, fine.

Rodsevich added 5 commits December 6, 2024 01:57

removed erroneous entry from melos.yaml

2eba9e7

work with pubspec overrides for facilitating example package deps

1ec950d

order pubspec dependencies

b2312bc

support multiple Envied annotations (Fix petercinibulk#122)

8f9ba1f

bump version 1.0.1

22a2820

Rodsevich requested review from petercinibulk and techouse as code owners December 6, 2024 05:14

techouse changed the title ~~Fix #122~~ fix: multiple generations for same class Dec 10, 2024

techouse requested changes Dec 10, 2024

View reviewed changes

Rodsevich added 2 commits December 10, 2024 12:11

Revert "bump version 1.0.1"

9644c82

This reverts commit 22a2820.

revert to melos bs

00a4916

techouse requested changes Dec 10, 2024

View reviewed changes

techouse self-assigned this Dec 10, 2024

techouse added the enhancement New feature or request label Dec 10, 2024

dev instructions

bc0114e

techouse closed this Dec 10, 2024

techouse mentioned this pull request Dec 10, 2024

feat: multiple generations for same class #124

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: multiple generations for same class #123

fix: multiple generations for same class #123

Rodsevich commented Dec 6, 2024 •

edited

Loading

techouse Dec 10, 2024

techouse Dec 10, 2024

techouse Dec 10, 2024

techouse Dec 10, 2024

Rodsevich Dec 10, 2024

techouse Dec 10, 2024

techouse Dec 10, 2024

techouse Dec 10, 2024

techouse commented Dec 10, 2024

Rodsevich commented Dec 10, 2024

techouse commented Dec 10, 2024 •

edited

Loading

techouse Dec 10, 2024

techouse Dec 10, 2024

techouse Dec 10, 2024

Rodsevich commented Dec 10, 2024

techouse commented Dec 10, 2024

fix: multiple generations for same class #123

fix: multiple generations for same class #123

Conversation

Rodsevich commented Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techouse commented Dec 10, 2024

Rodsevich commented Dec 10, 2024

techouse commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rodsevich commented Dec 10, 2024

techouse commented Dec 10, 2024

Rodsevich commented Dec 6, 2024 •

edited

Loading

techouse commented Dec 10, 2024 •

edited

Loading