Homebrew / homebrew-cask

🍻 A CLI workflow for the administration of macOS applications distributed as binaries
https://brew.sh
BSD 2-Clause "Simplified" License
20.91k stars 10.7k forks source link

Get rid of `colon` #95207

Closed vitorgalvao closed 1 year ago

vitorgalvao commented 3 years ago

Per discussion bellow, the new method should be csv. Example:

version version.csv[0] version.csv[1] version.csv[2]
123,456,789 123 456 789

As per discussion in https://github.com/Homebrew/homebrew-cask-versions/pull/10064. : is a special character, and (unless I’m misremembering) it has given us trouble in the past. It’s used in highly specific cases, and those can be handled by multiple ,.

This issue both tracks what needs to be done and serves as discussion. To do this we’d need to:

In homebrew-cask * ~~[dissenter-browser](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/dissenter-browser.rb)~~ * ~~[synergy](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/synergy.rb)~~ * [x] [scala-ide](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/scala-ide.rb) * [x] [eclipse-dsl](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-dsl.rb) * [x] [chatmate-for-facebook](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/chatmate-for-facebook.rb) * [x] [xee](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/xee.rb) * [x] [focused](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/focused.rb) * [x] [oracle-jdk-javadoc](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/oracle-jdk-javadoc.rb) * [x] [eclipse-testing](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-testing.rb) * ~~[imazing-mini](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/imazing-mini.rb)~~ * [x] [dotnet-sdk](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/dotnet-sdk.rb) * [x] [eclipse-jee](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-jee.rb) * [x] [xiami](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/xiami.rb) * [x] [eclipse-modeling](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-modeling.rb) * [x] [noxappplayer](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/noxappplayer.rb) * [x] [eclipse-cpp](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-cpp.rb) * [x] [gemini](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/gemini.rb) * [x] [texpad](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/texpad.rb) * [x] [startupizer](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/startupizer.rb) * [x] [tiger-trade](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/tiger-trade.rb) * [x] [the-unarchiver](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/the-unarchiver.rb) * [x] [tower](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/tower.rb) * [x] [eclipse-ide](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-ide.rb) * [x] [cockatrice](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/cockatrice.rb) * [x] [captin](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/captin.rb) * [x] [chatmate-for-whatsapp](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/chatmate-for-whatsapp.rb) * [x] [dictionaries](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/dictionaries.rb) * [x] [eclipse-php](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-php.rb) * [x] [eclipse-java](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-java.rb) * [x] [oracle-jdk](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/oracle-jdk.rb) * [x] [eclipse-rcp](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-rcp.rb) * [x] [eclipse-installer](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-installer.rb) * [x] [texworks](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/texworks.rb) * [x] [eclipse-javascript](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/eclipse-javascript.rb) * [x] [dotnet](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/dotnet.rb) * ~~[fritzing](https://github.com/Homebrew/homebrew-cask/blob/master/Casks/fritzing.rb)~~
In homebrew-cask-versions * [x] [adoptopenjdk8](https://github.com/Homebrew/homebrew-cask-versions/blob/master/Casks/adoptopenjdk8.rb) * [x] [dotnet-sdk-preview](https://github.com/Homebrew/homebrew-cask-versions/blob/master/Casks/dotnet-sdk-preview.rb) * [x] [dotnet-preview](https://github.com/Homebrew/homebrew-cask-versions/blob/master/Casks/dotnet-preview.rb)
In homebrew-cask-drivers * ~~[brother-p-touch-update-software](https://github.com/Homebrew/homebrew-cask-versions/blob/master/Casks/brother-p-touch-update-software.rb)~~ * ~~[3dconnexion](https://github.com/Homebrew/homebrew-cask-versions/blob/master/Casks/3dconnexion.rb)~~ * ~~[insta360-studio](https://github.com/Homebrew/homebrew-cask-versions/blob/master/Casks/insta360-studio.rb)~~

Ping @Homebrew/cask

claui commented 3 years ago

Good points.

The only thing that bugs me is the version.after_comma.before_comma pattern that could emerge. I feel that’s quite a pain to write and read.

Would it be more contributor-friendly to have something like version.comma_parts[1]?

core-code commented 3 years ago

what exactly are the problems that the colon causes?

The only thing that bugs me is the version.after_comma.before_comma pattern that could emerge. I feel that’s quite a pain to write and read.

yes exactly. if we get rid of the colon i'd like to replace it with another character and not with multiple colons as that would less elegant and more work in many cases.

also, if there is a mass-replace of the colon please let me know the exact time this will be done beforehand - thanks!

vitorgalvao commented 3 years ago

The only thing that bugs me is the version.after_comma.before_comma pattern that could emerge. I feel that’s quite a pain to write and read.

Agreed, though not that much worse than the current state of affairs (.before_colon.after_comma). That still takes some mental effort.

Would it be more contributor-friendly to have something like version.comma_parts[1]?

I’m fine with that. It seems like the clearest of all solutions and we might even get away with just parts. version.parts[0] and version.parts[1] might even be mentally simpler to parse than before_comma and after_comma (which need to be mapped to the concept of “before” and “after”, which annoyingly are in the inverse alphabetical of what we want).

what exactly are the problems that the colon causes?

Before opening the issue I was under the impression we had gotten rid of it already, but I just realised I’m thinking of using /. That’s the thing that caused problems, but because HFS uses : for paths, I must’ve been thinking of it.

also, if there is a mass-replace of the colon please let me know the exact time this will be done beforehand

Worry not, I didn’t forget and I’m acutely aware these changes need to be handled with care. Though there aren’t that many casks that would need changing.

core-code commented 3 years ago

version.comma_parts[1]

if you do something new like that, could you give me a week of notice before changing any casks to use it?

claui commented 3 years ago

version.comma_parts[1]

if you do something new like that, could you give me a week of notice before changing any casks to use it?

Yes, absolutely.

vitorgalvao commented 3 years ago

Another reason why parts or comma_parts is superior to just getting rid of colon: after_comma.after_comma is weird. Even with one after_comma, it’s not obvious if that refers to the first or last.

core-code commented 3 years ago

how about using either - or _ as a replacement for : and add

after_hyphen
before_hyphen

after_underscore
before_underscore
core-code commented 3 years ago

also, do we have a specific case where the colon has ever produced a problem?

vitorgalvao commented 3 years ago

how about using either - or _ as a replacement for

Those are commonly used in versions, so it blurs the line between what’s a divider or not.

also, do we have a specific case where the colon has ever produced a problem?

Right now, I’m not sure. Though I’m getting partial to parts as a nicer method anyway. That said, I wouldn’t lose sleep over closing this, as long as we’re consistent.

reitermarkus commented 3 years ago

I think we should just have comma as a separator. Maybe version.csv[1] instead of version.parts[1]?

Example:

version "1.2.3,4567:9abcdef0"

url "#{version.after_comma.before_colon}"
url "#{version.before_colon.before_colon}"

vs.

version "1.2.3,4567:9abcdef0"

url "#{version.csv[1]}"

also, do we have a specific case where the colon has ever produced a problem?

The (mostly theoretical) technical issue:

You cannot have a path to a cask in a Caskroom with a version containing : in a PATH variable. At least I think so … in Finder you cannot even create a folder containing :.

The readability issue:

I guess mostly the problem is that having all of these combinations of #after_ and #before_ is hard to follow.

core-code commented 3 years ago

At least I think so … in Finder you cannot even create a folder containing :

i think this i just a Finder issue for compatibility with classic macOS - colon works normally in the filesystem / CLI

i agree that the proposed system will improve readability (and writability) in some cases though

vitorgalvao commented 3 years ago

So we have a record, pointing out a new place in the core that’s introducing :.

vitorgalvao commented 3 years ago

@Homebrew/cask Can we get a quick vote on if we should do this (👍) or close (👎)?

And if so, should we use comma_parts, parts, or csv? I’m fine with csv: it’s short and already maps well (comma-separated values, comma-separated version).

BrewTestBot commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

claui commented 3 years ago

I’m 👍 with csv.

SMillerDev commented 2 years ago

@bevanjkay with the new audit for this this issue can be closed right?

bevanjkay commented 2 years ago

The audit isn't 100% complete yet, I just haven't been able to get around to fixing it. At the moment it is only checking the url stanza, but it needs to check the entire cask_contents, or stanza by stanza. As far as I know there was only one cask using an alternative version pattern outside of the url.

sasconsul commented 1 year ago

I am not sure this is related, I get an error when trying to upgrade some outdated packages.

$ brew upgrade ==> Casks with 'auto_updates true' or 'version :latest' will not be upgraded; pass --greedy to upgrade them. ==> Upgrading 4 outdated packages: Error: Cask 'adoptopenjdk8' is unreadable: undefined method `before_colon' for "232:b09":Cask::DSL::Version Did you mean? before_comma

SMillerDev commented 1 year ago

Not unless you haven't updated for 3 years