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

Warn about publishing in mixed mode #2583

Merged
merged 6 commits into from
Jul 24, 2020

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Jul 23, 2020

@sigurdm sigurdm requested a review from jonasfj July 23, 2020 15:47
sigurdm added 2 commits July 24, 2020 10:04
The null-safety analysis now rejects any package that doesn't opt-in before
doing resolution. This is reflected in this test.
lib/src/solver/incompatibility.dart Outdated Show resolved Hide resolved
@@ -445,6 +446,17 @@ class Pubspec {
bool get isEmpty =>
name == null && version == Version.none && dependencies.isEmpty;

/// The language version implied by the sdk constraint.
///
/// Given no or unbounded constraint we assume language version 1.0.
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct?

Doesn't the spec say that the fallback language version is the whatever the current SDK has?

I can see that this might cause validation errors if you have no SDK constraint..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pub.dev treats packages with no constraint as dart 1 packages, so at least for null-safety checking I think this makes sense. We might have to revise later.

.languageVersion;
if (!rootLanguageVersion.supportsNullSafety) {
return NullSafetyAnalysisResult(
NullSafetyCompliance.notCompliant, 'Is not opting in to null safety');
Copy link
Member

Choose a reason for hiding this comment

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

Can we be more specific? Like "Package $packageId has not opted into null-safety with the environment.sdk constraint in pubspec.yaml", or something like it.

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 added SourceSpans highlighting the problem

final NullSafetyCompliance compliance;

/// `null` if compliance == [NullSafetyCompliance.compliant].
final String reason;
Copy link
Member

Choose a reason for hiding this comment

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

Love it, we might even want to output this in the JSON output from pub outdated.

final languageVersion = pubspec.languageVersion;
if (languageVersion == null || !languageVersion.supportsNullSafety) {
return NullSafetyAnalysisResult(NullSafetyCompliance.notCompliant,
'package:${dependencyId.name} is not opted into null safety');
Copy link
Member

Choose a reason for hiding this comment

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

Again, can we say something with environment.sdk in pubspec.yaml

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 added SourceSpans highlighting the problem

if (!languageVersion.supportsNullSafety) {
return NullSafetyAnalysisResult(
NullSafetyCompliance.notCompliant,
'$fileUrl is opting out of null safety');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'$fileUrl is opting out of null safety');
'$fileUrl is opting out of null-safety with language version marker $languageVersionToken');

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 added SourceSpans highlighting the problem

Copy link
Contributor Author

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

PTAL

lib/src/solver/incompatibility.dart Outdated Show resolved Hide resolved
.languageVersion;
if (!rootLanguageVersion.supportsNullSafety) {
return NullSafetyAnalysisResult(
NullSafetyCompliance.notCompliant, 'Is not opting in to null safety');
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 added SourceSpans highlighting the problem

final languageVersion = pubspec.languageVersion;
if (languageVersion == null || !languageVersion.supportsNullSafety) {
return NullSafetyAnalysisResult(NullSafetyCompliance.notCompliant,
'package:${dependencyId.name} is not opted into null safety');
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 added SourceSpans highlighting the problem

if (!languageVersion.supportsNullSafety) {
return NullSafetyAnalysisResult(
NullSafetyCompliance.notCompliant,
'$fileUrl is opting out of null safety');
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 added SourceSpans highlighting the problem

@sigurdm sigurdm merged commit 152e474 into dart-lang:master Jul 24, 2020
@sigurdm sigurdm deleted the warn_publish_mixed_mode branch July 24, 2020 15:06
@sigurdm sigurdm restored the warn_publish_mixed_mode branch July 24, 2020 15:06
@sigurdm sigurdm deleted the warn_publish_mixed_mode branch July 24, 2020 15:06
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.

3 participants