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

Deprecate any #16920

Merged
merged 29 commits into from
Feb 9, 2021
Merged

Deprecate any #16920

merged 29 commits into from
Feb 9, 2021

Conversation

juancarlospaco
Copy link
Collaborator

@juancarlospaco juancarlospaco commented Feb 3, 2021

  • Deprecate any.

note

when removing any to see what would break if any were removed instead of deprecated, only these packages are affected: chronicles, telebot, sam:

  nim c -o:chr -r chronicles.nim
  /home/runner/work/Nim/Nim/pkgstemp/chronicles/chronicles.nim(161, 31) Error: undeclared identifier: 'any'
  nim c -o:tbot -r src/telebot.nim
  /home/runner/.nimble/pkgs/sam-0.1.16/sam.nim(77, 24) Error: undeclared identifier: 'any'

(refs https://github.com/nim-lang/Nim/runs/1823138277)

@arnetheduck
Copy link
Contributor

would break if any were removed instead of deprecated, only these packages are affected:

er.. I'm not sure, but I believe you're not tracking all nim code out there?

Not that I mind deprecating bad ideas (in fact, I support it), but doing so goes against 1.0 guarantees / promises - what's the plan / next step after deprecation? It would be good to manage these expectations so that people that swallowed the "no breaking changes" pill know what's coming.

@juancarlospaco
Copy link
Collaborator Author

We are NOT removing any.

@timotheecour
Copy link
Member

timotheecour commented Feb 3, 2021

er.. I'm not sure, but I believe you're not tracking all nim code out there?

note that I wrote that line (see my edit in top post); obviously this only tracks important_packages.nim, which is a form of canary testing; that's the same for any deprecation introduced before this PR

as @juancarlospaco said this is not a breaking change. Deprecating is a necessary step to help users migrate their code, in particular if next nim version major bump (eg 2.x) turns those deprecations into errors/removes such symbols. Furthermore, it's trivial to ignore deprecation warnings in cmdline/user config (--warning:deprecated:off), or, pending timotheecour#403, in a more targetted way (--warning:deprecated:foo:on|off)

Breaking changes are sometimes the lesser evil to avoid accumulating cruft/bad design/error prone patterns and whether they're acceptable is decided on case by case basis; see nimLegacy* in codebase for examples of past breaking changes.

note

IMO a future PR that would introduce nimLegacyAny would also be acceptable:

when defined(nimLegacyAny):
  any* {.deprecated: "Deprecated since v1.5; Use auto instead.".} = distinct auto

(trivial to enable in your config; not all breaking changes are equally painful)

@juancarlospaco
Copy link
Collaborator Author

Can you review @xflywind so we get more reviews so is "mergeable". Thanks.

Copy link
Member

@ringabout ringabout left a comment

Choose a reason for hiding this comment

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

It doesn't break anything, looks good to me.

@timotheecour timotheecour merged commit 68ef0c6 into nim-lang:devel Feb 9, 2021
@timotheecour timotheecour deleted the kill-any branch February 9, 2021 03:21
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
@timotheecour timotheecour mentioned this pull request Jul 10, 2021
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.

deprecate any (redundant with auto)
4 participants