Skip to content

Conversation

jakemac53
Copy link
Contributor

@jakemac53 jakemac53 commented Sep 14, 2017

Also introduces the PUB_ALLOW_PRERELEASE_SDK environment variable which can override this behavior. Valid values are true (default), false, and quiet (true but no log message about the override).

Fixes #1692

@jakemac53 jakemac53 added the type-enhancement A request for a change that isn't a bug label Sep 14, 2017
return true;
break;
case "quiet":
warnAboutPreReleaseTwoDotZeroSdkOverrides = false;
Copy link
Member

Choose a reason for hiding this comment

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

PM nit...

This makes me sad. I hate mutable top-level things.

Could their be a private field w/ an enum type and then two public bool properties that just key off 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.

done (didn't use an enum though)

// Log output should mention the PUB_ALLOW_RELEASE_SDK environment
// variable and mention the foo and bar packages specifically.
output: allOf(contains('PUB_ALLOW_PRERELEASE_SDK'),
anyOf(contains('foo, bar'), contains('bar, foo'))),
Copy link
Member

Choose a reason for hiding this comment

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

Or you could just sort it? Might make it easier to parse for the user, too...

Copy link
Member

Choose a reason for hiding this comment

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

Or just:

allOf(contains('PUB_ALLOW_PRERELEASE_SDK'), contains('foo'), contains('bar'))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

went ahead and sorted it since this is actually a long list in many cases so its a bit easier to parse

Copy link
Member

@munificent munificent left a comment

Choose a reason for hiding this comment

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

I mostly reviewed for pub style and idioms. I'll let Kevin do the deep thinking around the actual semantics. :)

.join(', ');
if (overriddenPackages.isNotEmpty) {
log.message(log.yellow(
'Overriding Dart SDK constraint from <2.0.0 to <2.0.0-dev.infinity'
Copy link
Member

Choose a reason for hiding this comment

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

"SDK"

You did 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.

:D

final VersionRange _defaultUpperBoundSdkConstraint =
new VersionConstraint.parse("<2.0.0");

/// The upper bound contraint that matches the dev sdk.
Copy link
Member

Choose a reason for hiding this comment

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

"sdk", here and elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}();

/// Whether or not to warn about pre-release sdk overrides.
bool warnAboutPreReleaseTwoDotZeroSdkOverrides = true;
Copy link
Member

Choose a reason for hiding this comment

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

This returns the wrong value if it's read before allowPreReleaseTwoDotZeroSdk is. Like @kevmoo suggests, you probably want a single lazy-initialized backing field and then two bool getters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// Log output should mention the PUB_ALLOW_RELEASE_SDK environment
// variable and mention the foo and bar packages specifically.
output: allOf(contains('PUB_ALLOW_PRERELEASE_SDK'),
anyOf(contains('foo, bar'), contains('bar, foo'))),
Copy link
Member

Choose a reason for hiding this comment

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

Or just:

allOf(contains('PUB_ALLOW_PRERELEASE_SDK'), contains('foo'), contains('bar'))

});

test(
'pub doesn\'t log about pre-release sdk overrides if '
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think it's a little cleaner to use a double-quoted string instead of escaping the apostrophe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jakemac53 jakemac53 merged commit aad65fe into master Sep 14, 2017
@jakemac53 jakemac53 deleted the allow-prerelease-sdk-var branch September 14, 2017 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants