This repository has been archived by the owner on Jan 5, 2022. It is now read-only.
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.
Add CDS sources using the CLI #32
Add CDS sources using the CLI #32
Changes from 16 commits
1d33568
76c9726
b044751
aa8a579
b2493ac
d63eaf6
d32494f
0e99cf0
e550ec2
c3a1410
057ca68
cce04c3
653cfdd
22d318e
bb1a4ef
2991198
5b20a01
bce07bf
9b7a1af
e318054
bc2d643
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.
I don't think this is necessary. In
init
I added this as you may create a new directory. Here you need to have a dir already otherwise it doesn't make sense. That's why the--projectDir
is currently hidden (mainly there for tests and to quickly enable it if we find out it's needed).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.
But is it an issue? I would argue either we have it or we don't. So either remove the flag or keep both for consistency (and make it non-hidden, I guess). What do you think?
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.
My assumption was that a simpler (public) API makes it easier to understand the docs. So omitting a flag/arg that is not useful for 99% of users (and can be simply worked around by
cd projectDir
) would be a positive change.But I have not validated that assumption, so feel free to change my mind on this.
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 have removed all the
--projectDir
flags as discussed, but I decided to leave the argument, because I think it makes our commands consistent. I don't thinkprojectDir
makes the API hard to understand, it is still very simple, while the change is tiny.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.
To throw my 2cts in, I also have no idea whether we need it or not. So the question is how do find that out in the future? Otherwise we'll have the same discussion on the next PR.