Enhanced cvdr.md documentation#483
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
jemoreira
left a comment
There was a problem hiding this comment.
Thank you for your contribution! The documentation is certainly more useful now.
I left some minor comments, we can merge the PR once you have addressed them and signed the Google CLA (see comment from the review bot).
|
|
||
|
|
||
| ```bash | ||
| CVDR_USER_CONFIG_PATH=/path/to/cvdr.toml ./cvdr --help |
There was a problem hiding this comment.
We should also make cvdr try to read config files from the default locations when the variables are not defined.
There was a problem hiding this comment.
Is it the current behavior I can document, or is it on a TODO list?
There was a problem hiding this comment.
My original comment wasn't relevant at all, sorry about that. Let me try again:
These example cvdr invocations should apply to anyone who built cvdr from source or installed it via debian package. When cvdr is installed in your system the CVDR_USER_CONFIG_PATH is not necessary and including it here and in the other examples could lead to confusion. Instead, add a step to the instructions on how to build it from source to create the following alias:
alias cvdr="CVDR_USER_CONFIG_PATH=/path/to/cvdr.toml $(realpath ./cvdr)"
That way all the examples using just cvdr work for all the ways in which cvdr could have been installed.
For context: When cvdr is installed via debian package it's not copied directly to /usr/bin/cvdr, but somewhere else not in the $PATH. /usr/bin/cvdr is just a shell script that defines environments variables (like config path and http proxies) and does some checks before executing the "real" cvdr binary.
There was a problem hiding this comment.
I see now, thanks.
I addressed it in the docs, but I wonder why not define the default config file paths in code rather than a bash wrapper, so that both the package and the manual builds can read them by default.
Added expected output for successful build process.
|
Thank you for review! Addressed your comments. Did the CLA agreement (though not very happy about it). |
|
Please use a rebase instead of a merge when updating the branch. You can fix the existing merge commit when you squash the extra commits after the PR is approved. |
Sure, I can do it. But can't we just rely on automatic squash on merge? |
We use a merge queue, which is configured to keep your commits and rebase them on top of main. We preserve individual commits from PRs because that let us revert them individually too. |
Got it, thank you! Guess it's less relevant for a single-file docs PR, but I will remember for future contributions. |
Enhanced cvdr documentation, made it more ad-hoc to newcomers:
As a newcomer, it was pretty difficult to get along with the command without reading the code. Opened this PR to make it easier for other people.