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

formula: add deprecation_date and disable_date methods #10108

Merged
merged 2 commits into from
Dec 24, 2020
Merged

formula: add deprecation_date and disable_date methods #10108

merged 2 commits into from
Dec 24, 2020

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Dec 23, 2020

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

This PR adds the Formula#deprecation_date and Formula#disable_date methods. These will return the respective dates when they exist. If not, the methods return nil. This is needed for Homebrew/actions#133 and Homebrew/homebrew-core#67386.

I'm marking this as critical because it is blocking those other PRs. Since the PR is a simple change that only adds methods that aren't used in the codebase yet, I don't feel the full 24 hours are needed. However, I will wait to merge until I'm ready to move forward with the other PRs.

@Rylan12 Rylan12 added the critical Critical change which should be shipped as soon as possible. label Dec 23, 2020
@BrewTestBot
Copy link
Member

BrewTestBot commented Dec 23, 2020

Review period ended.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

I'm fine with this but I wonder if as-well/instead it'd be easier for the actions to have this be part of the JSON output? I'd imagine @EricFromCanada would perhaps also use it for formulae.brew.sh. No worries if not (it's not blocking).

# Returns `nil` if no date is specified.
# @!method deprecation_date
# @return Date
delegate deprecation_date: :"self.class"
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to have a delegate ... :"self.class" helper method because it's used in a tonne of places (as a follow-up PR).

Copy link
Member Author

@Rylan12 Rylan12 Dec 23, 2020

Choose a reason for hiding this comment

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

Agreed but I'll opt for the follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, thanks!

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 23, 2020

I'm fine with this but I wonder if as-well/instead it'd be easier for the actions to have this be part of the JSON output?

How would one get JSON information for all formulae in a ruby script? I know that brew info --json returns the JSON output and I could retrieve a ruby hash of the JSON output with formula.to_hash, but I don't think that solution works any better or worse than what I currently have.

Regardless, I think it makes sense to have the deprecation/disable date and reason in the JSON output, so I'll add it. That was Eric and others can use it if they desire.

@MikeMcQuaid
Copy link
Member

How would one get JSON information for all formulae in a ruby script? I know that brew info --json returns the JSON output and I could retrieve a ruby hash of the JSON output with formula.to_hash, but I don't think that solution works any better or worse than what I currently have.

I was thinking you might prefer shelling out to brew info --json than a Ruby script. If not: no worries.

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 23, 2020

I was thinking you might prefer shelling out to brew info --json than a Ruby script. If not: no worries.

Is there a significant speed/other benefit to doing that? We're already working in ruby so shelling out just to bring it back into ruby doesn't seem that helpful to me. Plus, speed isn't going to be an issue here. We only need the action to run once a day anyway so if it takes five minutes to run it's not a huge deal.

(I totally get that you're not pushing it I'm just curious why it might be better)

@MikeMcQuaid
Copy link
Member

Is there a significant speed/other benefit to doing that? We're already working in ruby so shelling out just to bring it back into ruby doesn't seem that helpful to me.

Nope, thought you might be shelling out from JS.

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 23, 2020

Nope, thought you might be shelling out from JS.

Ah, gotcha! In that case, makes a lot of sense.

@Rylan12 Rylan12 merged commit a8faeef into Homebrew:master Dec 24, 2020
@Rylan12 Rylan12 deleted the add-depreacte-disable-date branch December 24, 2020 00:36
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 23, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants