Skip to content

GeneratorBuilder flag for "part of" without "library". #179

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

Merged
merged 1 commit into from
Jun 22, 2017

Conversation

matanlurey
Copy link
Contributor

Closes #157.
Closes #138.

This approach will not lock-in all future users of source_gen to an unreleased 1.25.0. We can flip the default value in a future (breaking) release.

/cc @davidmorgan

@matanlurey matanlurey requested review from kevmoo and jakemac53 June 21, 2017 04:53
@matanlurey matanlurey added this to the v0.5.9 milestone Jun 21, 2017
@@ -33,6 +33,12 @@
```

In Dart SDK `>=1.25.0` this can be relaxed as `part of` can refer to a path.
To opt-in, `GeneratorBuilder` now has a new flag, `requireLibraryDirective`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an idea, but you can check the SDK version using Platform.version from dart:io, and we could make the default be based on that even? That would also likely require a dependency on pub_semver but that should be ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fwiw I actually like this idea, but it hasn't technically landed yet even at -dev. Maybe we can set the default based on that once 1.25.0 lands officially?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good - if it hasn't landed officially in any release there is no sdk constraint we can set either right.... so we can't really release this feature at all until then.

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 figured adding this flags gives users the chance to make that choice for themselves versus waiting for us to do the right thing ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I forgot about the fact that you need to update your sdk constraint if you are publishing these files as part of a package, so using it automatically is a no-go.

What we could do though is just add the sdk constraint in this package?

Copy link
Contributor

Choose a reason for hiding this comment

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

But ya in any case adding the flag seems like an easy workaround to sidestep the issue

@matanlurey matanlurey merged commit 924d485 into dart-lang:master Jun 22, 2017
@matanlurey matanlurey deleted the flag-library-required branch June 22, 2017 00:01
mosuem pushed a commit to dart-lang/build that referenced this pull request Dec 10, 2024
mosuem pushed a commit to dart-lang/build that referenced this pull request Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants