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 20164: print deprecation if a deprecated module is imported from a local function or a mixin. #10346

Conversation

FeepingCreature
Copy link
Contributor

There's two locations where this can come up, so I moved the import deprecation checking code into a helper in dmodule. Not sure if this is the right way to go or if there's a more natural place that's always checked when a module is imported; I couldn't find one offhand.

Ping @Geod24. Hi :)

@dlang-bot
Copy link
Contributor

dlang-bot commented Aug 24, 2019

Thanks for your pull request and interest in making D better, @FeepingCreature! 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
20163 normal Deprecated import in string mixin does not output diagnostic message
20164 normal Importing deprecated module at function-local scope does not output diagnostic message

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#10346"

/*******************************************
* Print deprecation warning if we're deprecated, when
* this module is imported from scope sc.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a DDoc "Params" statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@Geod24
Copy link
Member

Geod24 commented Aug 25, 2019

Thanks for tackling this! It is lacking tests though.

@FeepingCreature FeepingCreature force-pushed the fix/issue-20164-deprecated-module-import-in-function branch from 234b7e5 to e748850 Compare August 25, 2019 09:22
@FeepingCreature
Copy link
Contributor Author

Oops! Forgot to git add the test :)

@FeepingCreature FeepingCreature force-pushed the fix/issue-20164-deprecated-module-import-in-function branch from e748850 to 2b76a3b Compare August 25, 2019 09:56
@FeepingCreature
Copy link
Contributor Author

Added test for 20163 too.

@Geod24
Copy link
Member

Geod24 commented Aug 25, 2019

Last nit: Your commit should reference both issues so that it appears in the changelog.
Otherwise LGTM

@FeepingCreature FeepingCreature force-pushed the fix/issue-20164-deprecated-module-import-in-function branch 2 times, most recently from 3401b1b to 0c2dadb Compare August 25, 2019 12:20
@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Aug 25, 2019

Didn't know you could reference both. Fixed. Ddoc syntax too.

Is it okay for the 20163 test to use the 20164 import?

…is imported from a local function or a mixin.
@FeepingCreature FeepingCreature force-pushed the fix/issue-20164-deprecated-module-import-in-function branch from 0c2dadb to f202a72 Compare August 25, 2019 15:57
@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Aug 25, 2019

Bot didn't pick up the issue number. Trying a different syntax...

edit: Now it got it.

edit: Buildkite failure seems random.

@dlang-bot dlang-bot merged commit f39f2eb into dlang:master Aug 26, 2019
@FeepingCreature FeepingCreature deleted the fix/issue-20164-deprecated-module-import-in-function branch August 26, 2019 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants