bottles: translate foo@1.2 to fooAT1.2

[DomT4]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you successfully run brew tests with your changes locally?

---

Guess who hates special characters? Bintray hates special characters! Need to do the translation here as well so things like openssl@1.1 don't cause Bintray to reject our upload.

Follow up to #812.

[DomT4]

Coverage decreased (-0.004%) to 57.889%

Oh, come on 😆.

[UniqMartin]

This isn't doing what it is supposed to do:

$ brew irb
==> Interactive Homebrew Shell
Example commands available with: brew irb --examples
irb(main):001:0> Utils::Bottles::Bintray.package("gcc")
=> nil
irb(main):002:0> Utils::Bottles::Bintray.package("gcc@6.2")
=> "gccAT6.2"
irb(main):003:0> Utils::Bottles::Bintray.package("crf++")
=> nil

You'll need to either append a sub (without bang) to the tr or introduce a local variable:

formula_name.to_s.tr("+", "x").sub(/(.)@(\d)/, "\\1AT\\2")

# or

package_name = formula_name.to_s.dup
package_name.tr!("+", "x")
package_name.sub!(/(.)@(\d)/, "\\1AT\\2")
package_name

# or

package_name = formula_name.to_s.tr("+", "x")
package_name.sub!(/(.)@(\d)/, "\\1AT\\2")
package_name

(I'm favoring the 1st variant, that can be split across multiple lines to improve readability, if that's desired.)

[DomT4]

Thanks. I did actually test this a bit, but I managed to confuse myself by doing:

fname = Formula["wget"].name
fname.to_s.sub!(/(.)@(\d)/, "\\1AT\\2")
=> nil
fname
=> "wget"

I managed to miss the fname part wouldn't be automatically triggered.

[UniqMartin]

Guess who hates special characters? Bintray hates special characters!

What is the actual error message? Could it be that an @ is perfectly valid in a package name on Bintray, but we simply fail to properly escape this character when constructing the Bintray API request URL? If not, are you aware of a list of valid characters that can be used in Bintray package names? The reason I'm asking all these questions is that the full-caps AT in the package name looks fairly ugly (to me). Once we start using it, it will be difficult to impossible to revise this decision. Maybe there's a better replacement character we can use, like x for +.

Coverage decreased (-0.004%) to 57.889%

Oh, come on 😆.

If you felt like addressing this, it should be fairly straightforward to add some unit tests for the methods in Utils::Bottles::Bintray. Given that the current implementation of this PR is broken, that's a strong indicator we would benefit from those tests.

[DomT4]

Could it be that an @ is perfectly valid in a package name on Bintray,

It's not, I checked the website:

Package name can only contain letters, numbers and the following characters: .-_:

I do occasionally follow up on curious errors 😉.

If you felt like addressing this, it should be fairly straightforward to add some unit tests for the methods in Utils::Bottles::Bintray. Given that the current implementation of this PR is broken, that's a strong indicator we would benefit from those tests.

I'll take a peek, but I'm not convinced coverage isn't a little broken. This PR failed coverage tests & all I was doing there is tweaking existing lines from http to https 😕.

[UniqMartin]

I do occasionally follow up on curious errors 😉.

Thanks! I didn't doubt you did, just wanted to gently nudge you to share your findings with us (error messages you saw, relevant resources you found, …).

From the listed characters, would you consider : a viable replacement character for @ on Bintray? I think openssl:1.1 reads a lot nicer than opensslAT1.1, : isn't currently used in formula names, and it isn't a valid character in a formula (class) name.

I'll take a peek, but I'm not convinced coverage isn't a little broken.

The change in coverage makes sense here. You added a line to a method that wasn't covered, so the number of relevant lines increased, but the number of covered lines did not, hence the (very small) drop in coverage.

This PR failed coverage tests & all I was doing there is tweaking existing lines from http to https 😕.

This is a different case as far as I can tell. The latest Travis job for the PR failed very early before it could run the test suite and generate the coverage report. The Travis job that was supposed to run after the merge is still pending. Or am I overlooking something?

[DomT4]

From the listed characters, would you consider : a viable replacement character for @ on Bintray?

I guess I don't have a strong objection as long as you don't mind the inconsistency of it being opensslAT11 in the formula, openssl@1.1 for the filename & openssl:1.1 on Bintray. I like consistency but I'm not overly fussed here if you've a strong positive feeling for it.

The Travis job that was supposed to run after the merge is still pending. Or am I overlooking something?

There was a coverage/coveralls — job in the PR, which failed, IIRC. The Travis problems started after that was merged, I think. Could be wrong though!

[DomT4]

: for bottles in place, tests added.

[chdiza]

FWIW, the colon is weird.

If the file names for bottles in HOMEBREW_CACHE are going to have this colon, they get presented by the Finder as having a slash in place of the colon. I think that looks unacceptably weird in a filename on a Mac.

If the file names of bottles in the cache don't have the colon---if that's just what they're called on Bintray's servers---that seems fine.

[DomT4]

If the file names of bottles in the cache don't have the colon---if that's just what they're called on Bintray's servers---that seems fine.

{"openssl@1.1":{"formula":{"pkg_version":"1.1.0","path":"Library/Taps/homebrew/homebrew-core/Formula/openssl@1.1.rb"},"bottle":{"root_url":"https://homebrew.bintray.com/bottles","prefix":"/usr/local","cellar":"/usr/local/Cellar","rebuild":0,"tags":{"el_capitan":{"filename":"openssl@1.1-1.1.0.el_capitan.bottle.tar.gz","sha256":"1c387846400ee0d03713c935f1ce14f558dfa48c0d18b9146cb52246828637c4"}}},"bintray":{"package":"openssl:1.1","repository":"bottles"}}}

[MikeMcQuaid]

👍 to this PR. Would be good (if we haven't already) to specifically audit (or actually block) formulae now with AT, : or @ to avoid 💥 (but can be another PR).

[DomT4]

Would be good (if we haven't already) to specifically audit (or actually block) formulae now with AT, : or @ to avoid 💥 (but can be another PR).

I'm having a slow day. Can you clarify this as well? 🙈. I think you're saying you want formulae audited to make sure people are using AT in the classname instead of @ or :, but I'm not 💯 on that belief.

[MikeMcQuaid]

@DomT4 Yeh, make sure they are using valid name formats in both the class name, formula name and filename.

[UniqMartin]

@DomT4 I guess better late than never: Thanks for adding some tests and for giving some thought to my suggestions and eventually incorporating some of them. Much appreciated! ❤️