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

info: change disabled dates #18551

Closed
wants to merge 1 commit into from
Closed

info: change disabled dates #18551

wants to merge 1 commit into from

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented Oct 11, 2024

  • Remove useless REMOVE_DISABLED_BEFORE
  • Add deprecated_period attribute to be able to finetune the disable date
  • This new attribute is set to :long by default to be backward compatible
  • This will allow to write an automated script that will disable the formula after the right amount of time

This also fixes the issue that the disabled date displayed in brew info right now
is way to far in the future (1 year) instead of 6 months (either it's a bug or the code does not aligne with the doc)

  • 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?

@iMichka iMichka force-pushed the disable branch 2 times, most recently from 674f539 to 1345056 Compare October 12, 2024 21:31
@iMichka iMichka marked this pull request as draft October 12, 2024 21:31
@iMichka
Copy link
Member Author

iMichka commented Oct 12, 2024

I will add some test if the idea is accepted.

Also, I am open for a better wording :)

- Remove useless REMOVE_DISABLED_BEFORE
- Add deprecated_period attribute to be able to finetune the disable date
- This new attribute is set to :long by default to be backward compatible
- This will allow to write an automated script that will disable the formula after the right amount of time

This also fixes the issue that the disabled date displayed in brew info right now
is way to far in the future (1 year) instead of 6 months (either it's a bug or the code does not aligne with the doc)
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.

Thanks @iMichka! I think one month is too short. I'm not opposed to the DSL but if it's just 3 vs. 6 months I wonder if it helps us much?

@Bo98
Copy link
Member

Bo98 commented Oct 13, 2024

I don't get the point of the DSL when we can just disable! date: XXXX in advance already.

I think the automatic disable date is problematic. If we know the date in advance we can just add it to the formula directly? We shouldn't be promising dates we are unable to keep.

If that information wasn't available at the time of deprecation then it likely means the formula is complicated and needed a longer indeterminate deprecation period for some reason.

@cho-m
Copy link
Member

cho-m commented Oct 13, 2024

I don't get the point of the DSL when we can just disable! date: XXXX in advance already.

In most situations, it probably makes sense to just do this. Maybe want to specify in documentation that it is preferred.

One minor quirk is that the instant you add disable!, then the formula is considered deprecated and thus the message ends up like

brew info [email protected] | rg disable
Deprecated because it is not maintained upstream! It will be disabled on 2028-04-01.

Which will probably confuse users.

@iMichka
Copy link
Member Author

iMichka commented Oct 13, 2024

The current code does also some wrong thing IMHO:

brew info cowsay
-> It will be disabled on 2024-11-22.

So there is 1 year of deprecated state (but our doc says we can go faster than this), and then 1 year of disabled state. I noticed that most of formulae need 2 years to go from first deprecation to removal. That's a very long time for most of them.

I don't get the point of the DSL when we can just disable! date: XXXX in advance already.

Maybe we could get rid of the deprecate! DSL then, to simplify things?

One minor quirk is that the instant you add disable!, then the formula is considered deprecated and thus the message ends up like

This is also something that should be easy to improve.


To summarise, I would like to

  1. improve / automatise the current workflow more
  2. bring it on line with our docs (and look if we can shorten the docs by replacing the long description by code)
  3. fix the It will be disabled on 2028-04-01. which is often wrong

Not saying my implementation is the best here, I'm open for other ideas

@carlocab
Copy link
Member

carlocab commented Oct 14, 2024

I don't get the point of the DSL when we can just disable! date: XXXX in advance already.

Agreed. I suggest deprecating the deprecate! DSL (lol).

If we can't commit to a disable date, then we probably shouldn't be deprecating a formula.

@MikeMcQuaid
Copy link
Member

I think the automatic disable date is problematic. If we know the date in advance we can just add it to the formula directly? We shouldn't be promising dates we are unable to keep.
...
If we can't commit to a disable date, then we probably shouldn't be deprecating a formula.

I couldn't disagree more, I'm afraid.

Dates are useful for people to be able to plan against. If the date is "wrong" then this is still vastly more useful than doing a rug pull on people with no meaningful advance notice.

Many people (and projects) do not pay attention to deprecations that they don't have dates for. Ideally we'd have dates for our own odeprecated too.

I don't get the point of the DSL when we can just disable! date: XXXX in advance already.

Maybe we could get rid of the deprecate! DSL then, to simplify things?

I'd rather we kept it around.

One minor quirk is that the instant you add disable!, then the formula is considered deprecated and thus the message ends up like

This is also something that should be easy to improve.

Agreed. It would be good to fix this.

To summarise, I would like to

  1. improve / automatise the current workflow more
  2. bring it on line with our docs (and look if we can shorten the docs by replacing the long description by code)
  3. fix the It will be disabled on 2028-04-01. which is often wrong

I agree with 1. and 2. of these high-level goals 👍🏻. I think the dates should be ideally used for automation but, if they are wrong and we deprecate/disable/remove after that date: no big deal. We should make sure we don't remove things before their stated dates.

@carlocab
Copy link
Member

I think the automatic disable date is problematic. If we know the date in advance we can just add it to the formula directly? We shouldn't be promising dates we are unable to keep.
...
If we can't commit to a disable date, then we probably shouldn't be deprecating a formula.

I couldn't disagree more, I'm afraid.

Dates are useful for people to be able to plan against. If the date is "wrong" then this is still vastly more useful than doing a rug pull on people with no meaningful advance notice.

I agree. But I'm not sure how keeping deprecate! helps with this. In practice, most deprecations occur immediately, so users almost always have no notice of future deprecations. We also don't currently display any messaging about the handful of future deprecations that we do have. So, as is, deprecate! isn't much use, whereas disable! is useful:

  • it automates removal of the formula one year after the given disable! date
  • it shows users notice that the formula is deprecated currently, and it gives them a date for when it will be disabled.

@MikeMcQuaid
Copy link
Member

In practice, most deprecations occur immediately, so users almost always have no notice of future deprecations.

Most, not all. Additionally: it's fine for deprecations to occur immediately for end-users because it's just a warning for them.

Something like deprecate!\n disable! date: seems fine and we can stop dating deprecations if it's literally useless for us to do so.

A deprecate_and_later_disable! (terrible name) or something might be a better DSL here. I think disable! reads as "disable right now", something we usually don't want.

@carlocab
Copy link
Member

carlocab commented Oct 14, 2024

A deprecate_and_later_disable! (terrible name) or something might be a better DSL here. I think disable! reads as "disable right now", something we usually don't want.

Note that this is equivalent to the current disable!. That is, doing something like

disable! date: "2024-12-31", because: "reason"

deprecates the formula immediately and then tells the user that it will be disabled on 2024-12-31.

@MikeMcQuaid
Copy link
Member

deprecates the formula immediately and then tells the user that it will be disabled on 2024-12-31.

Yeh. I don't think that's a great DSL because it don't think it reads as being deprecated right now. Maybe I care about this and no-one else does, though 😁.

Might be nice to at least have something like:

disable! date: "2024-12-31", deprecate_date: "2024-06-31", "because: "reason"

(with potentially better naming).

If we had something like that that was obvious enough: yeh, I think we could consider deprecating deprecate!.

@Bo98
Copy link
Member

Bo98 commented Oct 14, 2024

Dates are useful for people to be able to plan against. If the date is "wrong" then this is still vastly more useful than doing a rug pull on people with no meaningful advance notice.

The current situation is: we are providing them a rug and pulling it early nearly every time (but no maintainer was aware until user reports came in as visibility was poor). So "wrong" can be harmful as we've given them a date to plan against that we never intended to meet.

Even the 6 month thing hasn't been universal. We applied 3 months for the Python libraries and agreed on that before starting the deprecation process.

A deprecate_and_later_disable! (terrible name) or something might be a better DSL here. I think disable! reads as "disable right now", something we usually don't want.

This could work by changing one line of code (@deprecated = true -> @deprecated = true if @deprecation_date.nil? in disable! or similar):

deprecate! date: "2025-01-01"
disable! date: "2026-01-01", because: :abc

@cho-m
Copy link
Member

cho-m commented Oct 14, 2024

deprecate! date: "2025-01-01"
disable! date: "2026-01-01", because: :abc

Seems reasonable to me as may as well retain the original dates. We already have entries in JSON API which align to them

curl -sL "https://formulae.brew.sh/api/formula/[email protected]" | jq 'with_entries(select(.key | startswith("deprecat") or startswith("disabl")))'
{
  "deprecated": true,
  "deprecation_date": null,
  "deprecation_reason": "unsupported",
  "disabled": false,
  "disable_date": "2024-11-21",
  "disable_reason": null
}

Also would help webpage, e.g. https://formulae.brew.sh/formula/[email protected]#default

This formula has been deprecated since because it is not supported upstream

@cho-m
Copy link
Member

cho-m commented Oct 30, 2024

This could work by changing one line of code (@deprecated = true -> @deprecated = true if @deprecation_date.nil? in disable! or similar):

Needs a couple more lines of changes and AST is currently disable! first, but maybe something like #18661. Quick mock up so haven't checked if it handles all scenarios correctly

@Bo98
Copy link
Member

Bo98 commented Oct 30, 2024

This could work by changing one line of code (@deprecated = true -> @deprecated = true if @deprecation_date.nil? in disable! or similar):

Needs a couple more lines of changes and AST is currently disable! first, but maybe something like #18661. Quick mock up so haven't checked if it handles all scenarios correctly

Ah ok - my asumption was deprecate! first.

Probably worth functionally supporting both orders even we end up preferring a specific order cosmetically - taps aren't required to run brew style. An option if we don't want to do that is making it error on misuse.

@iMichka
Copy link
Member Author

iMichka commented Nov 7, 2024

Closing in favour of #18661. I'm fine with having the possibility to use both together to bring clarity.
At least this should reduce the manual "deprecate -> disable" work we have to do.

There are still a few tweaks I would like to make to the doc but we can discuss that later.

@iMichka iMichka closed this Nov 7, 2024
@iMichka iMichka deleted the disable branch November 7, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants