silence spurious warning in arsd package #2683
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ PR OK, no changes in deprecations or warnings Total deprecations: 14 Total warnings: 0 Build statistics: statistics (-before, +after)
executable size=5272064 bin/dub
rough build time=95sFull build output |
WebFreak001
requested changes
Aug 9, 2023
|
I don't think that's possible in general without possibly affecting other packages inappropriately. (tbh idk why this warning exists at all, but presumably it has some purpose for somebody) |
|
well it makes sense since otherwise the package source files are not importable by other packages - if you have special dflags to allow that, silence the warning if any such dflag is specified |
|
Well, why does it only give this on dub test anyway? Maybe this rootPackage check will do it well enough. |
|
The CI is green @WebFreak001 @Geod24 can you take a look at this one-line change? |
WebFreak001
requested changes
Aug 11, 2023
|
Why not? Why warn about the package definition of a dependency? You (as the user of a dependency with no access to the dependency's repo in general anyway) aren't going to fix it, so it is unactionable spam. |
|
Consider the following: If the warning is important, it being accurate is important. The dflags cannot be accurate in the general case, since it is freeform and cannot be validated by dub. Checking my package name is a more robust test, it will never have a false negative the way a fuzzy string check on dflags would. If the warning is not important, it shouldn't be there. |
If you make your own package, with your own sub-packages, you want to see the warnings since you are the one who resolves them. If you find them in a random dub package, you have to report an issue to the maintainers. what we should really be doing here in the long term:
I'm not a fan of hardcoding package names into dub, it will stay there for probably years and the package may change in a way after which the warning might actually make sense. Although since it's only a warning, it's not a major issue and I think for the short term we could just do that. In that case, better revert to check your hardcoded package name instead of "is root package" @s-ludwig what do you think? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This warning doesn't apply to my package since it tells the compiler through dflags instead of importPaths where to find the module, but there is no other obvious way to silence it.
If it only triggered when running dub test in my own packages, I'd just ignore it, but dub test on outside third party packages that merely depend on mine trigger these too, so I want it silenced somehow.