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

Validate git is clean when publishing #4373

Merged
merged 4 commits into from
Sep 10, 2024

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Sep 5, 2024

Fixes #2040

Does not warn about newly added files, because they might get generated by a build step.

@sigurdm sigurdm requested review from szakarias and jonasfj September 5, 2024 13:15
import '../utils.dart';
import '../validator.dart';

/// A validator that validates that no checked in files are modiofied in git.
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
/// A validator that validates that no checked in files are modiofied in git.
/// A validator that validates that no checked in files are modified in git.

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

class GitStatusValidator extends Validator {
@override
Future<void> validate() async {
if (package.inGitRepo) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: please return early instead of nesting a bit more 🤣

Suggested change
if (package.inGitRepo) {
if (package.inGitRepo) {
return;
}

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

Comment on lines 46 to 49
for (var i = 0; i < output.length; i++) {
if (output[i] != 0) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps consider splitting this out into a helper library.. git.parseMachineStatus or something.

This might be worth having unit tests cover.

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 split something out - but it is hard to make something really general I think...

if (output[i] != 0) {
continue;
}
final filename = utf8.decode(
Copy link
Member

Choose a reason for hiding this comment

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

you might want to handle FormatException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Done!

/// The first [skipPrefix] bytes of each substring will be ignored (useful for
/// `git status -z`). If there are not enough bytes to skip, throws a
/// [FormatException].
List<Uint8List> splitZeroTerminated(Uint8List output, {int skipPrefix = 0}) {
Copy link
Member

Choose a reason for hiding this comment

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

splitZeroTerminated without the skipPrefix might have been more sane.

But this is also fine :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - more sane - just I wanted to avoid creating too much garbage, and (as far as I can tell) utf8.decode cannot work on a partial buffer...

@sigurdm sigurdm merged commit bfbc6bc into dart-lang:master Sep 10, 2024
23 checks passed
srawlins added a commit to srawlins/dartdoc that referenced this pull request Oct 2, 2024
These files can have minor differences in them (depending on the version of
Dart?), so that it is difficult for them to always be identical to the ones
checked in, during CI. Then there is a new warning that `pub publish --dry-run`
produces, if the current git checkout is not clean:
dart-lang/pub#4373. We need to reset these files to
avoid that warning.
srawlins added a commit to dart-lang/dartdoc that referenced this pull request Oct 3, 2024
…sh (#3896)

These files can have minor differences in them (depending on the version of
Dart?), so that it is difficult for them to always be identical to the ones
checked in, during CI. Then there is a new warning that `pub publish --dry-run`
produces, if the current git checkout is not clean:
dart-lang/pub#4373. We need to reset these files to
avoid that warning.
@MaryaBelanger
Copy link

Hi @sigurdm, couple questions for docs:

  • Does the warning show when using --dry-run?
  • Does --skip-validation omit the warning?

@sigurdm
Copy link
Contributor Author

sigurdm commented Dec 3, 2024

Hi @sigurdm, couple questions for docs:

  • Does the warning show when using --dry-run?

Yes, all the Validators are run for a dry-run

  • Does --skip-validation omit the warning?

Also yes. No Validators are run when --skip-validation is passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn in pub publish if git status is not clean
3 participants