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

Fix Issue 20015 - [REG 2.086] Deprecated -preview, -revert, and -transition options not documented #13784

Merged
merged 1 commit into from
Mar 15, 2022

Conversation

RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented Mar 8, 2022

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 8, 2022

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
20015 regression [REG 2.086] Deprecated -preview, -revert, and -transition options not documented

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + dmd#13784"

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 8, 2022

cc @ibuclaw

Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

Test[s]?

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 8, 2022

I didn't add any because, currently, all deprecated features have their documentation purged and the fix is obviously correct.

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 8, 2022

Furthermore, I don't see any tests for cli switches in the fail compilation directory. I understand where you come from @Geod24 , but I think that in this situation we can just merge this as is.

@MoonlightSentinel
Copy link
Contributor

See

Shouldn't some of these tests fail with this PR?

@Geod24
Copy link
Member

Geod24 commented Mar 10, 2022

I didn't add any because, currently, all deprecated features have their documentation purged and the fix is obviously correct.

Side note: The main point of test is not to ensure that things work today (although it's still a pretty important side of it), but to ensure that they will work tomorrow. This is especially important for regression, because we have evidence of something that used to work that doesn't anymore.

Furthermore, I don't see any tests for cli switches in the fail compilation directory.

https://github.com/dlang/dmd/blob/master/test/fail_compilation/reserved_version_switch.d
And obviously what @MoonlightSentinel linked, although those are in the "compilable" directory.

@RazvanN7
Copy link
Contributor Author

@Geod24 At this point there is no deprecated switch with public documentation in dmd. There is a deprecated transition (complex) and a deprecated preview (intpromote) but both have their public documentation disabled. One thing I could do is to enable their documentation and list them as deprecated. If that is not acceptable, then I don't know how I could possibly test this without introducing a mock-up deprecation. What do you think?

@RazvanN7
Copy link
Contributor Author

@Geod24 The -transiton=complex is deprecated but it is not listed in the documentation. To be able to test this I marked it as documented and deprecated. Is that ok? I don't really understand what the strategy is here; my expectation was that once a switch is deprecated it is also purged from the documentation to avoid new uses of it (that would require a single Feature field, not 2), but it seems that it is possible to have an undocumented switch which is not deprecated (like preview=markdown on the stable branch). This seems weird to me, but I am obviously missing context here.

@RazvanN7
Copy link
Contributor Author

cc @MoonlightSentinel

Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

Yes, that test is enough IMO

@Geod24
Copy link
Member

Geod24 commented Mar 15, 2022

This seems weird to me, but I am obviously missing context here.

It's good that the default was enabled but the switch became a no-op (and not deprecated). That's what we did for -dip25 too. This way, early adopters still get the behavior they expect, without needing to version their code to remove an annoying deprecation at a specific version.

@dlang-bot dlang-bot merged commit 53f9e4a into dlang:stable Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants