search

dart-lang / code_builder

A fluent API for generating valid Dart source code
https://pub.dev/packages/code_builder
BSD 3-Clause "New" or "Revised" License
423 stars 66 forks source link

Add support for super initializer constructor arguments #368

Closed yanok closed 1 year ago

srawlins commented 1 year ago

@jakemac53 would you be able to review this, or are you comfortable with me reviewing it?

jakemac53 commented 1 year ago

If you don't mind also removing the invariant_booleans lint from the analysis_options.yaml file, that will get the rest of the tests running.

yanok commented 1 year ago

This generally LGTM but we also need an update to the version and an entry in the changelog.

I added an entry to the changelog but do you really bump the version for every change? Or did I understand it incorrectly?

yanok commented 1 year ago

If you don't mind also removing the invariant_booleans lint from the analysis_options.yaml file, that will get the rest of the tests running.

I disabled the lint, but now it fails with 2.12 version...

jakemac53 commented 1 year ago

I added an entry to the changelog but do you really bump the version for every change? Or did I understand it incorrectly?

What you added is fine - we do want to list all user visible changes in the changelog. But see my note on the number, since were are already in a dev release you can keep that same dev release version.

I disabled the lint, but now it fails with 2.12 version...

I am not fully understanding the output of that failing test, but super parameters were not supported until 2.17.0. We may want to bump the min SDK for this package to that version? That probably has something to do with the failure.

If you update that you will need to update the min version we run tests on in the .github/workflows/test-package.yml file.

yanok commented 1 year ago

What you added is fine - we do want to list all user visible changes in the changelog. But see my note on the number, since were are already in a dev release you can keep that same dev release version.

Yes, that's what I thought about the dev version. Rolled back the version bump.

I am not fully understanding the output of that failing test, but super parameters were not supported until 2.17.0. We may want to bump the min SDK for this package to that version? That probably has something to do with the failure.

It was surprising that it fails, since the test just generates the text, but then I realized we probably run dartfmt on the generated code, it breaks since it can't parse super parameter, so the code stays unformatted and that breaks the test. I bumped the minimal SDK version but it's a pity we have to do this just for tests to work.

jakemac53 commented 1 year ago

cc @srawlins should we go ahead and release this package or are we waiting on anything else?

srawlins commented 1 year ago

Not waiting on anything else, I think