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

[core] Stop handling beta, nightly #922

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

confused-Techie
Copy link
Member

This PR ended up being much simpler than expected.
But it's goal was remove any explicit handling of beta or nightly for Pulsar in reference to release channels.

Since Pulsar has not used that nomenclature for over a year, it seemed past due time.

In it's place, I have added dev, since for our dev instance of Pulsar that is what it's currently recognized as, which makes sense since that version is only accessible when literally in dev mode.

What may be worth mentioning is this change doesn't address the already non-existent distinction between rolling and regular release channels. But that wasn't handled previously so this is no change in that respect. Here we just remove what's become useless code.

@Daeraxa
Copy link
Member

Daeraxa commented Feb 13, 2024

I assume the rest of the code that referenced nightly and beta was in the auto update code that we pruned a while ago.

@confused-Techie
Copy link
Member Author

Ahh @Daeraxa you are probably right there, so makes sense why there was so little still referencing it

Comment on lines 609 to 612
// Public: Returns a {Boolean} that is `true` if the current version is an official release.
isReleasedVersion() {
return this.getReleaseChannel().match(/stable|beta|nightly/) != null;
return this.getReleaseChannel().match(/stable|dev/) != null;
}
Copy link
Member

Choose a reason for hiding this comment

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

I dunno, you can build Pulsar locally without changing the version string in package.json from e.g. 1.113.0-dev. I don't generally think -dev versions are "Released Versions"... Right?

Comment on lines 47 to 56
getCommandNameForChannel(commandName) {
let channelMatch = this.appVersion.match(/beta|nightly/);
let channelMatch = this.appVersion.match(/dev/);
let channel = channelMatch ? channelMatch[0] : '';

switch (channel) {
case 'beta':
return `${commandName}-beta`;
case 'nightly':
return `${commandName}-nightly`;
case 'dev':
return `${commandName}-dev`;
default:
return commandName;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a situation where Pulsar can be launched with the CLI command name pulsar-dev?

Otherwise, I think we might just need to reduce this function's body to simply return commandName...

@confused-Techie
Copy link
Member Author

@DeeDeeG You are absolutely correct, in that the changes adding dev are made for in development Pulsar versions. Not ever released versions.

I thought it'd be akin to beta but you are right that there's a distinct difference between a released version and everything else. If anything we could change this to detect rolling releases and non-rolling releases. But for the purpose of this PR do you think this logic should be even more simplified?

@DeeDeeG
Copy link
Member

DeeDeeG commented Feb 15, 2024

I reckon that, coincidentally for each of these separately, it makes the most sense to remove beta and nightly without replacing them with anything.

(Tangent: If we do remove handling for beta and nightly that is, since who knows if some fork would want to roll those release channels, and being able to handle them here doesn't actively hurt anything much. But this would clean up a few lines of code and run perhaps some nanoseconds faster, I dunno.)

@DeeDeeG
Copy link
Member

DeeDeeG commented Feb 15, 2024

FWIW I don't see us using getCommandNameForChannel() anywhere internally in core repo, so if it's not meant to be a public API, or usable by packages, etc., we can delete the function entirely. [EDIT to add: I tried and I can't do atom.getCommandNameForChannel() in Pulsar Dev Tools, so it seems like it's not a public API. Might as well delete the function entirely if it's unused?]

isReleasedVersion() is clearly marked as being a public API, maybe some packages use it. But I don't see us currently using it anywhere ourselves either, at least not internally in core repo. So, I think we can't delete this without it technically being semver major / breaking (?).

But by and large these functions are both kind of obsolete without there being the full-on "release channels" concept supported by us anymore, where we flavor/style the app based on the channel name.

@Spiker985
Copy link
Member

FWIW I don't see us using getCommandNameForChannel() anywhere internally in core repo, so if it's not meant to be a public API, or usable by packages, etc., we can delete the function entirely. [EDIT to add: I tried and I can't do atom.getCommandNameForChannel() in Pulsar Dev Tools, so it seems like it's not a public API. Might as well delete the function entirely if it's unused?]

I think that function was entirely used internally via the update scripts previously. I don't think there's any use for it outside of that

isReleasedVersion() is clearly marked as being a public API, maybe some packages use it. But I don't see us currently using it anywhere ourselves either, at least not internally in core repo. So, I think we can't delete this without it technically being semver major / breaking (?).

Could probably just give it the Grim deprecate treatment and remove it in a version or two? (Since there are larger problems with the way that Atom was set up to be a 1.y.z-only application we can't ever do a true Semver major)

@DeeDeeG
Copy link
Member

DeeDeeG commented May 8, 2024

We could put a grim notice on it, but the fact we can never actually do a 2.0 means I think we should plan that there is effectively no deadline for package authors to act on the grim notifications... unless we intend to do small breaking things in the same semver major, which I don't think is worth it. So it's kind of strange to deprecate things and also plan to never remove them. I think we just make isReleasedVersion() a very simple function and not deprecate, as there is no sincere intent to remove... right?

@DeeDeeG
Copy link
Member

DeeDeeG commented May 8, 2024

I'd be willing to push changes to the branch simplifying the functions as per above feedback if that would be okay, at which point I believe this would be ready to merge IMO.

EDIT: This assuming no objections from other reviewers either of course.

@confused-Techie
Copy link
Member Author

@DeeDeeG If you have the time to push changes to get this one ready to go that would be totally fine by me, and I'd appreciate it!

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.

4 participants