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

Feat: Deprecate warnings #361

Merged
merged 2 commits into from
Oct 12, 2020
Merged

Feat: Deprecate warnings #361

merged 2 commits into from
Oct 12, 2020

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Oct 9, 2020

based on discussion here: #285 (comment)

Comment on lines +627 to +632
-- function utils.deprecation_warning(msg, trace)
-- io.stderr:write(msg .. "\n" .. trace .."\n")
-- end
function utils.deprecation_warning(msg, trace)
-- this does nothing by default
end
Copy link
Member

Choose a reason for hiding this comment

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

I assume the idea here is only to do nothing by default until the next major release (and override for our own testing), but starting in the next major release we will start throwing our internal deprecation warnings by default (and let people override to disable), correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, see the note regarding overriding this function in applications, not in libraries. I think it might be better to not have a default implementation, and upon a release that removes stuff add a proper warning to the change log.

Just to make sure we do not accidentally break (other peoples) stuff.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is any use is having a deprecation warning feature that is off by default and people have to define a function to get access to it. Half the point is to ease the pain of upgrades for people that aren't paying any attention to them at all! Especially if the dependency for their project is indirect, there is no way an appreciable percentage of users are going to set up a deprecation warning catcher in their systems. What we want to do is warn them before their stuff breaks unexpectedly. In order to do that this must be on by default. Yes that might break a few people's setups if they relly on STDERR streams for some reason to function, but that's a lot less people than will be caught by unexpected deprecations they didn't have time to plan for because they never saw messages and don't read release notes closely for all dependencies.

Waiting until a major version to possible break STDERR by introducing these warnings is one thing, not enabling them by default ever is another (and I think nearly defeats the utility).

But yes, other libraries should never override this, apps may if they want to and do so intentionally. After that the onus is on them to update their old code or not update to a breaking release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Half the point is to ease the pain of upgrades for people that aren't paying any attention to them at all

That's a usecase for SemVer, not having to worry about it. But considering SemVer, we should not be breaking anything unless it is on major bump.
Generating additional output that wasn't there before breaks that promise. And hence might break stuff on minor or even patch releases.

So it would effectively mean that deprecation messages can only be added on major versions. That is also a very slow moving process which I'd like to avoid.

So effectively we can try to;

  • make users move fast, and Penlight slow
  • move Penlight fast, and let users deal with their own issues, whilst we provide the instruments

imo betting on other people to do what you want them to do usually doesn't work. Also the folks not paying attention are most likely also the ones that simply disable the warnings and move on.

Copy link
Member

@alerque alerque Oct 9, 2020

Choose a reason for hiding this comment

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

Generating additional output that wasn't there before breaks that promise.

I don't think changing the exact content of the STDERR stream output to add new warnings will be breaking a promise, only the position we are in now where we are changing what channels we do or don't output anything at all on is a breaking change (going from not using STDERR to using it for warnings). In this case I think the new promise for 2.0.0 and later is that STDERR is something Penlight can and will sometimes write warnings too. After that we're free to add new upcoming deprecation notices on whatever timeline we like as they come up and do the actual breaking removals on major releases.

People that disable warnings and move on and don't read changelogs can take care of themselves when something breaks. I have no pity for that. What I do what is a clear mechanism by which they would have been warned by default if they didn't explicitly choose their own adventure.

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 have a long history of breaking stuff in unexpected ways, because I assumed it would be fine. With #346 being the latest one in that chapter. Also at Kong we've had many problems from automatic version bumps on minor or patch level before, imo we should not risk breaking anything.

From a scoping perspective; the problem of downstream projects keeping in sync or falling behind on Penlight versions, is not a Penlight problem to solve.

So far in this discussion I changed my mind from not sure to a firm no. @hishamhm what is your opinion on this? input appreciated.

@alerque wrt this PR, code ok for pre-2.0.0?

Copy link
Member

@alerque alerque Oct 10, 2020

Choose a reason for hiding this comment

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

My opinion is firming up too — in the opposite direction! 😋

In the meanwhile I don't see anything in here that is a problem to merge pre-2.0.0. Since we at least both agree that we can't start enabling warnings on a hitherto unused IO channel before 2.0.0 anyway this is the way the code needs to be until then.

Only when releasing 2.0.0 should we announce what our promise is: either to never add or change deprecation warning messages on STDERR except on major releases (your way, requires 2 major releases for every deprecation, one to announce future deprecation and one to actually do it) OR to clarify that STDERR can and will be used by default for new deprecation warnings on minor releases announcing upcoming major versions but can be handled via the override function as well (my way, requires 1 major release bump when actual breaking change takes place).

Copy link
Member Author

Choose a reason for hiding this comment

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

your way, requires 2 major releases for every deprecation, one to announce future deprecation and one to actually do it

no, that's not what I'm advocating. Options so far are;

  1. do not add a default warning mechanism, deprecate in major, minor, even patch. Removal is on the next major. This allows Penlight to move fast.
  2. add a default warning mechanism, but deprecate only on major. Removal is on next major. This is slow moving.
  3. add a default warning mechanism. Deprecate in major, minor, even patch. Removal is on next major. This is also fast moving for Penlight.

My preferred option is 1, yours is 3 if I'm correct.

1 and 2 are safe imo, 3 requires to exclude the stderr output from the versioning scope (to be SemVer compliant)

@Tieske Tieske merged commit 0400d57 into master Oct 12, 2020
@Tieske Tieske deleted the deprecate branch October 12, 2020 22:07
bungle added a commit to Kong/kong that referenced this pull request May 6, 2021
### Summary

 - deprecate: `permute.iter`, renamed to `permute.order_iter` (removal later)
   [#360](lunarmodules/Penlight#360)
 - deprecate: `permute.table`, renamed to `permute.order_table` (removal later)
   [#360](lunarmodules/Penlight#360)
 - deprecate: `Date` module (removal later)
   [#367](lunarmodules/Penlight#367)
 - feat: `permute.list_iter` to iterate over different sets of values
   [#360](lunarmodules/Penlight#360)
 - feat: `permute.list_table` generate table with different sets of values
   [#360](lunarmodules/Penlight#360)
 - feat: Lua 5.4 'warn' compatibility function
   [#366](lunarmodules/Penlight#366)
 - feat: deprecation functionality `utils.raise_deprecation`
   [#361](lunarmodules/Penlight#361)
 - feat: `utils.splitv` now takes same args as `split`
   [#373](lunarmodules/Penlight#373)
 - fix: `dir.rmtree` failed to remove symlinks to directories
   [#365](lunarmodules/Penlight#365)
 - fix: `pretty.write` could error out on failing metamethods (Lua 5.3+)
   [#368](lunarmodules/Penlight#368)
 - fix: `app.parse` now correctly parses values containing '=' or ':'
   [#373](lunarmodules/Penlight#373)
 - fix: `dir.makepath` failed to create top-level directories
   [#372](lunarmodules/Penlight#372)
 - overhaul: `array2d` module was updated, got additional tests and several
   documentation updates
   [#377](lunarmodules/Penlight#377)
 - feat: `aray2d` now accepts negative indices
 - feat: `array2d.row` added to align with `column`
 - fix: bad error message in `array2d.map`
 - fix: `array2d.flatten` now ensures to deliver a 'square' result if `nil` is
   encountered
 - feat: `array2d.transpose` added
 - feat: `array2d.swap_rows` and `array2d.swap_cols` now return the array
 - fix: `aray2d.range` correctly recognizes `R` column in spreadsheet format, was
   mistaken for `R1C1` format.
 - fix: `aray2d.range` correctly recognizes 2 char column in spreadsheet format
 - feat: `array2d.default_range` added (previously private)
 - feat: `array2d.set` if used with a function now passes `i,j` to the function
   in line with the `new` implementation.
 - fix: `array2d.iter` didn't properly iterate the indices
   [#376](lunarmodules/Penlight#376)
 - feat: `array2d.columns` now returns a second value; the column index
 - feat: `array2d.rows` added to be in line with `columns`
bungle added a commit to Kong/kong that referenced this pull request May 6, 2021
### Summary

 - deprecate: `permute.iter`, renamed to `permute.order_iter` (removal later)
   [#360](lunarmodules/Penlight#360)
 - deprecate: `permute.table`, renamed to `permute.order_table` (removal later)
   [#360](lunarmodules/Penlight#360)
 - deprecate: `Date` module (removal later)
   [#367](lunarmodules/Penlight#367)
 - feat: `permute.list_iter` to iterate over different sets of values
   [#360](lunarmodules/Penlight#360)
 - feat: `permute.list_table` generate table with different sets of values
   [#360](lunarmodules/Penlight#360)
 - feat: Lua 5.4 'warn' compatibility function
   [#366](lunarmodules/Penlight#366)
 - feat: deprecation functionality `utils.raise_deprecation`
   [#361](lunarmodules/Penlight#361)
 - feat: `utils.splitv` now takes same args as `split`
   [#373](lunarmodules/Penlight#373)
 - fix: `dir.rmtree` failed to remove symlinks to directories
   [#365](lunarmodules/Penlight#365)
 - fix: `pretty.write` could error out on failing metamethods (Lua 5.3+)
   [#368](lunarmodules/Penlight#368)
 - fix: `app.parse` now correctly parses values containing '=' or ':'
   [#373](lunarmodules/Penlight#373)
 - fix: `dir.makepath` failed to create top-level directories
   [#372](lunarmodules/Penlight#372)
 - overhaul: `array2d` module was updated, got additional tests and several
   documentation updates
   [#377](lunarmodules/Penlight#377)
 - feat: `aray2d` now accepts negative indices
 - feat: `array2d.row` added to align with `column`
 - fix: bad error message in `array2d.map`
 - fix: `array2d.flatten` now ensures to deliver a 'square' result if `nil` is
   encountered
 - feat: `array2d.transpose` added
 - feat: `array2d.swap_rows` and `array2d.swap_cols` now return the array
 - fix: `aray2d.range` correctly recognizes `R` column in spreadsheet format, was
   mistaken for `R1C1` format.
 - fix: `aray2d.range` correctly recognizes 2 char column in spreadsheet format
 - feat: `array2d.default_range` added (previously private)
 - feat: `array2d.set` if used with a function now passes `i,j` to the function
   in line with the `new` implementation.
 - fix: `array2d.iter` didn't properly iterate the indices
   [#376](lunarmodules/Penlight#376)
 - feat: `array2d.columns` now returns a second value; the column index
 - feat: `array2d.rows` added to be in line with `columns`
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 13, 2021
## 1.10.0 (2021-04-27)

 - deprecate: `permute.iter`, renamed to `permute.order_iter` (removal later)
   [#360](lunarmodules/Penlight#360)
 - deprecate: `permute.table`, renamed to `permute.order_table` (removal later)
   [#360](lunarmodules/Penlight#360)
 - deprecate: `Date` module (removal later)
   [#367](lunarmodules/Penlight#367)
 - feat: `permute.list_iter` to iterate over different sets of values
   [#360](lunarmodules/Penlight#360)
 - feat: `permute.list_table` generate table with different sets of values
   [#360](lunarmodules/Penlight#360)
 - feat: Lua 5.4 'warn' compatibility function
   [#366](lunarmodules/Penlight#366)
 - feat: deprecation functionality `utils.raise_deprecation`
   [#361](lunarmodules/Penlight#361)
 - feat: `utils.splitv` now takes same args as `split`
   [#373](lunarmodules/Penlight#373)
 - fix: `dir.rmtree` failed to remove symlinks to directories
   [#365](lunarmodules/Penlight#365)
 - fix: `pretty.write` could error out on failing metamethods (Lua 5.3+)
   [#368](lunarmodules/Penlight#368)
 - fix: `app.parse` now correctly parses values containing '=' or ':'
   [#373](lunarmodules/Penlight#373)
 - fix: `dir.makepath` failed to create top-level directories
   [#372](lunarmodules/Penlight#372)
 - overhaul: `array2d` module was updated, got additional tests and several
   documentation updates
   [#377](lunarmodules/Penlight#377)
 - feat: `aray2d` now accepts negative indices
 - feat: `array2d.row` added to align with `column`
 - fix: bad error message in `array2d.map`
 - fix: `array2d.flatten` now ensures to deliver a 'square' result if `nil` is
   encountered
 - feat: `array2d.transpose` added
 - feat: `array2d.swap_rows` and `array2d.swap_cols` now return the array
 - fix: `aray2d.range` correctly recognizes `R` column in spreadsheet format, was
   mistaken for `R1C1` format.
 - fix: `aray2d.range` correctly recognizes 2 char column in spreadsheet format
 - feat: `array2d.default_range` added (previously private)
 - feat: `array2d.set` if used with a function now passes `i,j` to the function
   in line with the `new` implementation.
 - fix: `array2d.iter` didn't properly iterate the indices
   [#376](lunarmodules/Penlight#376)
 - feat: `array2d.columns` now returns a second value; the column index
 - feat: `array2d.rows` added to be in line with `columns`


## 1.9.2 (2020-09-27)

 - fix: dir.walk [#350](lunarmodules/Penlight#350)


## 1.9.1 (2020-09-24)

 - released to superseed the 1.9.0 version which was retagged in git after some
   distro's already had picked it up. This version is identical to 1.8.1.

## 1.8.1 (2020-09-24) (replacing a briefly released but broken 1.9.0 version)

## Fixes

  - In `pl.class`, `_init` can now be inherited from grandparent (or older ancestor) classes. [#289](lunarmodules/Penlight#289)
  - Fixes `dir`, `lexer`, and `permute` to no longer use coroutines. [#344](lunarmodules/Penlight#344)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants