Skip to content

Feature: skip-temp-tag#620

Merged
evocateur merged 6 commits intolerna:masterfrom
noherczeg:feature/skip-temp-tag
Feb 28, 2017
Merged

Feature: skip-temp-tag#620
evocateur merged 6 commits intolerna:masterfrom
noherczeg:feature/skip-temp-tag

Conversation

@noherczeg
Copy link
Copy Markdown
Contributor

I have no idea if this is a proper solution, therefore I'd like to have some guidance on how to properly test this.

@noherczeg
Copy link
Copy Markdown
Contributor Author

In response to #293

Comment thread src/commands/PublishCommand.js Outdated
});
if (skipTempTag) {
this.progressBar.tick(pkg.name);
this.execScript(pkg, "postpublish");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where is the actual publish happening with --skip-temp-tag?

@noherczeg
Copy link
Copy Markdown
Contributor Author

noherczeg commented Feb 24, 2017

So the idea is that
If we ignore temp tagging then:

  • npmPublishAsPrerelease() should immediately set the proper tag value, and
  • npmUpdateAsLatest() should skip the updateTag() procedure which handles temp tag removal, and addition of the proper tags

If we did not ignore temp tagging then:

  • npmPublishAsPrerelease() will publish with the "lerna-temp" tag, while
  • npmUpdateAsLatest() will trigger the updateTag() procedure...

@noherczeg
Copy link
Copy Markdown
Contributor Author

I also introduced the getDistTag() method which might be an overkill, so if you guys feel like it I could remove it. I made it because as it stands in the code I kind of needed something like that in multiple places.

}

npmUpdateAsLatest(callback) {
const {skipTempTag} = this.getOptions();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we've skipped temp-tagging, why not just avoid the runParallelBatches call entirely?

const {skipTempTag} = this.getOptions();

if (skipTempTag) {
  return callback();
}

this.progressBar.init(...)

Copy link
Copy Markdown
Contributor Author

@noherczeg noherczeg Feb 27, 2017

Choose a reason for hiding this comment

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

Yeah, I missed that. Corrected in 0c6ac7e

Comment thread src/commands/PublishCommand.js Outdated

getDistTag() {
const {npmTag, canary} = this.getOptions();
return npmTag ? npmTag : canary ? "canary" : "latest";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I really dislike nested ternaries, especially all on one line. Wouldn't boolean defaults work here?

return npmTag || (canary && "canary") || "latest";

Comment thread src/commands/PublishCommand.js Outdated
npmPublishAsPrerelease(callback) {
const {skipTempTag} = this.getOptions();
// if we skip temp tags we should tag with the proper value immediately therefore no updates will be needed
const tag = !skipTempTag ? "lerna-temp" : this.getDistTag();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's invert the ternary (const tag = skipTempTag ? this.getDistTag() : "lerna-temp";) and make sure there's a newline between this assignment and the next forEach block.

@evocateur
Copy link
Copy Markdown
Member

LGTM, thanks for your responsiveness @noherczeg!

If @gigabo has no objections, I will merge this tomorrow.

@lock
Copy link
Copy Markdown

lock Bot commented Dec 27, 2018

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock Bot locked as resolved and limited conversation to collaborators Dec 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants