-
-
Notifications
You must be signed in to change notification settings - Fork 232
dub run <package> should search the registry on failure #1428
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
Conversation
Thanks for your pull request, @wilzbach! |
4e0836a
to
d9b5fde
Compare
BTW now that we show instructions on how to build a dlang PR locally: (example: dlang/dmd#8155) This would be even more interesting imho. |
Ping on this. I still think this would be nice to have and there haven't been any objections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a nice feature. I'd like to use this in scripts and such, but it won't be possible since it waits for user input. Can we have -y
command line option to assume "Yes" as answer to the prompt?
Good idea. Added. |
source/dub/commandline.d
Outdated
writefln("Description: %s", p.description); | ||
writefln("Version: %s", p.version_); | ||
|
||
bool answer = m_yes ? true : input("Do you want to fetch %s?".format(package_name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the change but I think we should move the m_yes/input method into a more general scope where all methods could read a boolean input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything else that currently uses this?
We could have easily done this after this PR has been merged and this simple improvement has been in the queue now for more than 1 1/2 years (see the original PR).
As dub run now implicit calls fetch, I wonder whether dub run should also have an argument "version". On the other hand one can say that is a special case for which I can use dub fetch && dub run. |
Ping @wilzbach what's the status of this? I'm very much in favor. |
This has been ready since three months except for the nitpick. We could always do that one later though. |
Rebased. |
This appears to break a bunch of shell script tests. |
E.g. test Maybe --single does not play well with your new logic. Therefore the tests fails which uses dub run --single file.d |
I just realized, with this feature the custom init idea from @MartinNowak (#600) becomes a low hanging fruit. Just a matter of rewriting the args[] array. |
95120bc
to
aeb7e0b
Compare
All green now. |
source/dub/commandline.d
Outdated
return 2; | ||
|
||
auto p = search.front.front; | ||
writefln("%s wasn't found locally, but it's available online:", package_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use std.stdio
directly, as this circumvents dub's logging infrastructure. Use logInfo
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm in dub init
std.stdio.writef
is used too.
source/dub/commandline.d
Outdated
auto pack = dub.packageManager.getFirstPackage(package_name); | ||
|
||
if (!pack) { | ||
bool input(string caption, bool default_value = true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is at least one more local definition of input
(e.g. for dub init
) in this file. I'd like if we could generalize this into a reusable free function + as @WebFreak001 mentioned the y/N
handling code.
Not a strong requirement from my side, just nice to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm the dub init
input
is quite different to this input as (1) a default value is allowed and (2) multiple values are allowed for a single value (all yes
, y
, 1
, YeS
, Y
, ...) map to 1
.
Though I'm not against unifying them by any means if someone wants to try it after this addition.
source/dub/commandline.d
Outdated
if (free_args.length < 1 || m_single) | ||
return super.execute(dub, free_args, app_args); | ||
|
||
const package_parts = free_args[0].split(":"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use splitPackageName
from #1643
c0ff469
to
ee84e96
Compare
5c12181
to
ea2f566
Compare
804679b
to
7c3ed45
Compare
Rebased, used We missed 2.085.0 by a day, but well at least we can get it in the next release. |
Ping @thewilsonator |
We already have a |
Well |
- to match .editoconfig and the rest of the code
(Reopened #1082 as I accidentally closed it during the push)
This has been pending in the queue for quite a while. Anything blocking this?