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

Remove unused imports [on hold]. #1948

Closed
wants to merge 3 commits into from
Closed

Remove unused imports [on hold]. #1948

wants to merge 3 commits into from

Conversation

veelo
Copy link
Contributor

@veelo veelo commented May 19, 2020

No description provided.

@dlang-bot
Copy link
Collaborator

Thanks for your pull request and interest in making D better, @veelo! 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.

@thewilsonator
Copy link
Contributor

source/dub/compilers/compiler.d(131,77): Error: no property toNativeString for type GenericPath!(PosixPathFormat)
source/dub/generators/sublimetext.d(56,39): Error: no property toNativeString for type GenericPath!(PosixPathFormat)

@WebFreak001
Copy link
Member

WebFreak001 commented May 19, 2020

@thewilsonator this kind of change needs a lot more checking, the CI output will probably not be enough here.

When removing imports the templates which aren't covered might break because they could have used symbols which are now removed. So for these changes you need to go through every template definition, version block, unittest block and debug block and look at all the used symbols for every file where an import is removed. If mixins are involved this gets a lot more work to check correctly too. Dub is used as a library too so something like this can easily cause issues.

(I'm just assuming here that you didn't do this thorough review because your approval is 2 minutes after the PR opened)

@veelo
Copy link
Contributor Author

veelo commented May 19, 2020

@WebFreak001 Thanks for pointing this out. I have grepped for "template" and "static if", which doesn't give hits in the files from this changeset. I just scanned the first 1k lines of source.dub.commandline.d without spotting an eponymous template, then I decided that I wouldn't have time to do the same for the rest...

@WebFreak001
Copy link
Member

WebFreak001 commented May 19, 2020

you can fairly reliably find function templates by searching for )(, but it won't be all templates and there will also be a few false positives.

Actually checking if the removed symbols are then really gone through that import is another challenge though. I looked at dependency.d where the dub.internal.vibecompat.inet.url import is possibly needed in line 279 but when looking at if toString is called it looks like that shouldn't actually be possible if the tests pass.

@veelo
Copy link
Contributor Author

veelo commented May 19, 2020

Actually checking if the removed symbols are then really gone through that import is another challenge though. I looked at dependency.d where the dub.internal.vibecompat.inet.url import is possibly needed in line 279 but when looking at if toString is called it looks like that shouldn't actually be possible if the tests pass.

I'm not sure I understand. You mean you would have expected compilation to fail because the template that uses the import is instantiated in the unittest? Adding to that, NativePath is used on line 52 of dub.dependency.d. I suspect dub.internal.vibecompat.inet.path is imported through some other public import, but I don't see it. I have tried running https://code.dlang.org/packages/depend on it, but couldn't get it to work.

I'm getting less enthusiastic about all this, maybe we should just close this PR... I don't understand the failing check either.

@veelo veelo changed the title Remove unused imports. Remove unused imports [on hold]. Jan 24, 2021
@veelo
Copy link
Contributor Author

veelo commented Jan 25, 2021

I have split this up into smaller PRs.

@veelo veelo closed this Jan 25, 2021
@Geod24
Copy link
Member

Geod24 commented Jan 25, 2021

Thanks!

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