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.
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
feat: add retrievetargetdir to source:retrieve #624
feat: add retrievetargetdir to source:retrieve #624
Changes from 6 commits
0091091
ededda5
2676119
c5ec4f6
77b7eb1
434b082
df8d554
b25ac34
a121b01
8bc1596
461b9e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
you can save a lot of fs ops by using the
withFileTypes
option onreaddir
https://nodejs.org/api/fs.html#fspromisesreaddirpath-options
which gives you the full dirEnt info https://nodejs.org/api/fs.html#class-fsdirent
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.
and what if I want to "peek" at the results, outside my normal project. So I say,
--retrievetargetdir peek/main/default
what's going to happen?
.replace(join('main', 'default')
would eliminate the redundancy?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'll give that a try, but my suspect that it will only remove one
main/default
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.
As predicted, the results land in
peek/main/default
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 feels like a good place for a warning. Another option would be to make this "overlaps" validation a standalone function that could be used for your new flag's
parse
so that it doesn't go through all that retrieve work to fail this late in the game.As is, it looks like the user would specify a targetDir and then if it overlapped, the retrieve would go to their default dir anyway without a warning?
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.
@mshanemc I think I am going to change direction on overlapping targets, fail the command if the target overlaps a package dir. There are other commands and variants of source:retrieve to get source into a package dir.
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.
overall comment...there's some tools that do this kind of thing if we find a lot more use cases for it.
https://github.com/supercharge/promise-pool is fairly small, zero-dep. The error handling and abort looks nice.
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'd omit this since it isn't used. It's not clear what it might do and at-a-glance invites an infinite loop
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.
@mshanemc it is used in mv function. Yes it can and yes I have :). Not being able to queue results defeats the ability to accomplish the dir traversal without recursion.
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.