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

docs: add deprecate/disable/removal documentation #10144

Merged
merged 8 commits into from Dec 29, 2020

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Dec 25, 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?

We don't have any official documentation about deprecating, disabling, and removing formulae. I've tried to document my thought process (which I think mirrors several that of several other maintainers). This is prompted by Homebrew/homebrew-core#66360 where @bayandin and @SMillerDev shared their thoughts. My thoughts aligned with theirs, so this PR is a conglomeration of our ideas.

I'd like to get feedback from other @Homebrew/core maintainers to make sure that everyone is on the same page about what's in the documentation. If I've written something that not everyone agrees with, we should come to a consensus to clarify expectations for all maintainers (and users).

@Rylan12 Rylan12 added discussion Input solicited from others documentation Documentation changes labels Dec 25, 2020
@Rylan12 Rylan12 requested a review from a team December 25, 2020 03:47
@BrewTestBot
Copy link
Member

Review period will end on 2020-12-28 at 03:47:53 UTC.

1 similar comment
@BrewTestBot
Copy link
Member

Review period will end on 2020-12-28 at 03:47:53 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 25, 2020
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
@carlocab
Copy link
Member

Thanks for this, @Rylan12!

Rylan12 and others added 2 commits December 25, 2020 00:07
Co-Authored-By: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
@SMillerDev
Copy link
Member

Can you add links to this in the formula cookbook as well, since that is supposed to be a complete list of syntax references IMHO.

docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved

If these pre-existing reasons do not fit, a custom reason can be specified. These reasons should be written to fit into the sentence `<formula> has been deprecated/disabled because it <reason>!`.

Here are examples of a well-worded custom reason:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we have to teach people grammar in the homebrew documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree this may be too much handholding but we do have a few issues currently in homebrew/core so I don't think this part should totally be removed.

See liblwgeom:

Warning: liblwgeom has been deprecated because it liblwgeom headers are not installed anymore, use librttopo instead!

And exomizer:

Error: exomizer has been disabled because it custom license!

Copy link
Member

Choose a reason for hiding this comment

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

I think a formula author/maintainer would reasonably expect:

  ... because: "<reason>"

to produce the message:

  [This is happening] because <reason>.

rather than:

  [This is happening] because it <reason>!

In the case of exomizer, "because it custom license" is just wrong, but "because custom license" accords with modern usage, though it's anathema for grammar pedants. 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

@gromgit I agree that this makes sense. This is how I initially created it, but at some point, it was decided that since most of the reasons already started with "it", the "it" should be added by default. See #8549 (comment) and Homebrew/homebrew-core#60321 (comment). TBH I think it could be changed, now that we have the symbols available.

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.

Some comments here but this is fantastics, great work @Rylan12!

docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
Co-Authored-By: Seeker <meaningseeking@protonmail.com>
Co-Authored-By: Sean Molenaar <smillerdev@me.com>
Co-Authored-By: Mike McQuaid <mike@mikemcquaid.com>
Co-Authored-By: Michka Popoff <3406519+iMichka@users.noreply.github.com>
@Rylan12
Copy link
Member Author

Rylan12 commented Dec 25, 2020

Thanks for the reviews, everyone!

I made some changes (I removed the highly contentious "serious issues" phrase).

There are a few comments that I left unresolved because I needed more clarification or wasn't quite in agreement with everyone involved.

docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
Co-Authored-By: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for adding this.

Co-Authored-By: Adrian Ho <the.gromgit@gmail.com>
@BrewTestBot
Copy link
Member

Review period ended.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 28, 2020
Co-Authored-By: Mike McQuaid <mike@mikemcquaid.com>
Co-Authored-By: Michka Popoff <3406519+iMichka@users.noreply.github.com>
Co-Authored-By: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
Co-Authored-By: Mike McQuaid <mike@mikemcquaid.com>
@Rylan12
Copy link
Member Author

Rylan12 commented Dec 28, 2020

Thanks, everyone!

@chenrui333
Copy link
Member

enabled auto-merge 5 hours ago

that is very nice!!

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 28, 2020

that is very nice!!

It would be even nicer if that final check would run... 😉😭

@Rylan12 Rylan12 merged commit bc4f555 into Homebrew:master Dec 29, 2020
@Rylan12 Rylan12 deleted the deprecate-disable-documentation branch December 29, 2020 14:28
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 28, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion Input solicited from others documentation Documentation changes outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants