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

Removed version from Slack application name #6375

Closed
wants to merge 1 commit into from
Closed

Removed version from Slack application name #6375

wants to merge 1 commit into from

Conversation

cafferata
Copy link
Contributor

Removed version from Slack application name

@tapeinosyne
Copy link
Contributor

As of 0.44.2, the application name still includes the version number. Is the cask outdated? (The appcast feed suggests that it is not.)

@cafferata
Copy link
Contributor Author

What exactly do you mean? According git diff the version is successfully removed

@tapeinosyne
Copy link
Contributor

Sorry, I was unclear. The app bundle, as downloaded, is named Slack 0.44.2.app, and thus interpolation of #{version} is necessary; without it, brew cask install will fail.

@cafferata
Copy link
Contributor Author

Solved my error, mistake! :-)

@cafferata
Copy link
Contributor Author

What is the reason that you closed this pull request without merging?

@rolandwalker rolandwalker reopened this Sep 29, 2014
@rolandwalker
Copy link
Contributor

Sorry, I thought I saw the phrase "my mistake", and thought you were withdrawing.

@tapeinosyne
Copy link
Contributor

I am not fond of mandating aesthetical decisions on all Cask users. However, we do not have a policy against it, and similar patches have been merged in the past.

@vitorgalvao, if your opinion differs, feel free to merge. Otherwise, I suggest to close this.

@vitorgalvao
Copy link
Member

@ndr-qef I agree. If the developer includes the version number, they must have a reason. Barring cases where not renaming could break things or when the apps name is inconsistent with its own branding (and hence confusing to users), we should leave them be.

That said, you’re also right in that we’ve merged similar cases in the past. I’ve done it (although I consider capitalising the first word in the name an absolutely minor change). Removing the version isn’t even a common change:

# removing version in app name
fwbuilder-#{version}.app => Firewall Builder.app
Wings3D #{version}.app => Wings3D.app

# other app renames
Bitcoin-Qt.app => Bitcoin Core.app
cockatrice.app => Cockatrice.app
oracle.app => Oracle.app
DEVONthink Pro.app => DEVONthink Pro Office.app
Flash Player.app => Flash Player Debugger.app
gingr-OSX64.app => Gingr.app
imagemin-app-v#{version}-darwin/Atom.app => imagemin.app
Nik Collection.app => Install Nik Collection.app
Liquifile_1_8.app => Liquifile.app
LogMeInIgnition.app => LogMeIn Client.app
MiniMetro-alpha13b-osx.app => Mini Metro.app
MusicManager.app => Music Manager.app
eclipse/Eclipse.app => Scala IDE.app
textsoap7.app => TextSoap.app

Particularly when comparing with the amount of casks we don’t remove the version number from:

BZFlag-#{version}.app
chirp-#{version}.app
CoqIde_#{version}.app
Crypt#{version}.app
djv-#{version}.app
DSP Radio #{version}.app
Free Ruler #{version}.app
Isabelle#{version}.app
Julia-#{version}.app
jxplorer-#{version}.app
KeyStore Explorer #{version}.app
Luminance HDR #{version}.app
Macdrops v#{version}.app
Mapture-#{version}.app
Mkvtoolnix-#{version}.app
Mudlet-#{version}.app
Pharo#{version}.app
Sage-#{version}.app
scilab-#{version}.app
Slack #{version}.app
TeXmacs-#{version}.app
UBER Network Fuser #{version}.app

Some of the renames in the first list are extremely questionable (like Music Manager) and should definitely not occur, while others (like Scala IDE) seem like they’re needed to not break things. I propose we revert most of them (including the capitalising ones I did), only leaving those that have reasons other than aesthetics to exist (this includes the previous two examples: preventing conflicts, and avoiding confusion). Some of them will be left with ugly names, but so be it, it’s the developer’s decision. Nothing is stopping users from manually renaming them, and if we ever implement alias, it’ll be even easier.

@ndr-qef @rolandwalker If you agree, I’ll close this and start an issue where I’ll see if I can fit the rule somewhere, and fix the relevant casks.

@rolandwalker
Copy link
Contributor

Agreed. Maybe best practice is for :target to get an explanatory comment. For example, as you referenced above, in bitcoin-core.rb, we renamed the target to resolve a specific ambiguity upstream. However, a reader looking at the source would have no way to know that.

@cafferata, thanks for your contribution. However, it seems that maintainers want to move away from this practice.

@vitorgalvao
Copy link
Member

@rolandwalker Adding an explanatory comment sounds good. I’m already taking it into account.

We’re all in agreement, so I’ll close this.

Small unrelated update: Music Manager actually has a good reason to be renamed, after all.

@cafferata
Copy link
Contributor Author

Thanks for the explanation(s)!

@cafferata cafferata deleted the slack branch October 1, 2014 07:43
rolandwalker added a commit to rolandwalker/homebrew-cask that referenced this pull request Oct 18, 2014
* update URL and switch to `:latest` version scheme
* more specific license
* remove `:target` used to change case, per Homebrew#6375

This is a weekly build.  Perhaps it belongs in the versions
repo with nightlies.
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 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.

4 participants