version: add empty? method

[samford]

• 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 written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

In livecheck, we currently have some code that compares a given Version to an empty version like this:

empty_version = Version.new("")
match_version_map.delete_if do |_match, version|
  next true if version == empty_version

It was suggested that we could refactor this to the following instead:

match_version_map.delete_if do |_match, version|
  next true if version.to_s.empty?

The goal here is to check if a Version is empty, so I figured it would be ideal if we could just do version.empty?. If this isn't an appropriate addition, I'm fine with closing this but I thought there may be some value here (i.e., not having to allocate an extra string just to use empty?).

Otherwise, if there are more appropriate locations for the code additions in version.rb and version_spec.rb, please let me know and I'll update this.

Edit: I had originally added both blank? and empty? methods but I learned that if you add empty? you automatically get blank? for free, so I updated this accordingly.

[MikeMcQuaid]

Looks good to me!

[MikeMcQuaid]

In livecheck, we currently have some code that compares a given Version to an empty version like this:

Actually: missed this. I'm OK with this rare case but in general: we should not be adding Homebrew/brew code to support brew livecheck. Instead, the brew livecheck code should be moved into Homebrew/brew.

[samford]

I'm OK with this rare case but in general: we should not be adding Homebrew/brew code to support brew livecheck. Instead, the brew livecheck code should be moved into Homebrew/brew.

Though adding Version.empty? benefits livecheck in this instance, I felt that it could potentially be useful in other contexts in the future. To my mind, this would still be true if this PR was created after livecheck is part of Homebrew/brew.

I don't think I'm properly understanding your meaning, so please correct me if I'm not grasping it. My intention isn't to debate your point here but simply to make sure I'm understanding you correctly, so I don't go against your expressed guideline.

---

If "brew livecheck code should be moved into Homebrew/brew" is referencing migrating brew livecheck into Homebrew/brew, we're actively moving in that direction. Nanda and I have been making changes to the core files in homebrew-livecheck to get them into a shape where brew livecheck can be reasonably migrated (e.g., working towards getting rid of some files that don't have an appropriate place in Homebrew/brew, reworking some files to fit in better with what's normal in Homebrew/brew, etc.). It's one of the explicit goals of the GSoC project and I can guarantee it will be done sometime before GSoC is over (end of August).

The refactoring mentioned above (next true if Version.empty? now that this is merged) was suggested in a larger PR that significantly refactors some of homebrew-livecheck's core files to bring them more in line with Homebrew/brew (while also enabling subsequent changes in livecheck, like the oft-referenced strategy feature). There's one more bit of restructuring that needs to be done (moving the contents of heuristic.rb into other files and deleting it) and then brew livecheck should be in better shape for migration to Homebrew/brew. There will still need to be changes during the process but I believe we'll have done all we can beforehand to make the process less of a headache.

Sorry if I've misunderstood your meaning here and needlessly volunteered this information.

[MikeMcQuaid]

If "brew livecheck code should be moved into Homebrew/brew" is referencing migrating brew livecheck into Homebrew/brew, we're actively moving in that direction.

Yup, knew this and it's great 👏 👍.

Just making the point that, for future changes like this, it'd probably be best to make them in homebrew-livecheck and then port them over when it's merged in.

[samford]

Just making the point that, for future changes like this, it'd probably be best to make them in homebrew-livecheck and then port them over when it's merged in.

Thanks for explaining. That makes total sense to me and I've already been planning to take that approach in some upcoming changes.

The final PR for the strategy feature will appear soon (after a number of preceding PRs making changes to enable the feature on a technical level) and it's necessary to add it to the livecheck DSL in the process. However, the strategy feature depends on livecheck_strategy.rb to translate strategy symbols to LivecheckStrategy classes, so it's not technically possible to add it to Homebrew/brew's livecheck.rb file until we do the migration to Homebrew/brew anyway. Instead, I'll be creating an extension of livecheck.rb in homebrew-livecheck where I'll modify only the parts that are necessary to add strategy to the DSL.

I discussed this with Nanda not too long ago and decided that we should just include any changes that need to be added into the livecheck DSL into the homebrew-livecheck copy and wait to integrate the changes during the migration. I'll make sure that we do this for all changes that impact Homebrew/brew going forward, so thanks for the feedback.