Various minor fixes and documentation #17
Merged
Conversation
This file contains hidden or 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
Maintainers are expected to only modify flake.nix. While the url url of the completion script isn't expected to change (unless Tarides adopts the project), the goal of this change is to make it clear to maintainers where the completion script comes from. Signed-off-by: Stephen Sherratt <[email protected]>
Passing `completion = null;` to dune.nix causes the dune package to be built without a completion script. Signed-off-by: Stephen Sherratt <[email protected]>
Signed-off-by: Stephen Sherratt <[email protected]>
shonfeder
approved these changes
Aug 21, 2025
README.md
Outdated
| tag of Dune included in the release. | ||
|
|
||
| Dune does not contain a mechanism for generating its own completion script. The | ||
| completion script is generated using a semi-manual process. Dune use the |
Member
There was a problem hiding this comment.
Suggested change
| completion script is generated using a semi-manual process. Dune use the | |
| completion script is generated using a semi-manual process. Dune uses the |
README.md
Outdated
| completion script is generated using a semi-manual process. Dune use the | ||
| [cmdliner](https://github.com/dbuenzli/cmdliner) library for its CLI which | ||
| currently lacks the ability to generate shell completion scripts. To generate | ||
| shell completion scripts for Dune, replace its usage of cmdliner with an |
Member
There was a problem hiding this comment.
Suggested change
| shell completion scripts for Dune, replace its usage of cmdliner with an | |
| shell completion scripts for Dune, we replace its usage of cmdliner with an |
README.md
Outdated
|
|
||
| ## Completion Script | ||
|
|
||
| [This repo](https://github.com/gridbugs/dune-completion-scripts) contains bash |
Member
There was a problem hiding this comment.
I initially thought 'this' referred to this repo, so
Suggested change
| [This repo](https://github.com/gridbugs/dune-completion-scripts) contains bash | |
| The [gridbugs/dune-completion-scripts repo](https://github.com/gridbugs/dune-completion-scripts) contains bash |
Comment on lines
+179
to
+180
| updated in place which is not ideal. Currently the install script doesn't | ||
| attempt to verify the checksum of the binary distro but if we ever add that |
Member
There was a problem hiding this comment.
I guess we should probably do this before long? It seems like best practice right? Maybe we could wait until someone asks for it tho?
Contributor
Author
There was a problem hiding this comment.
It is the best practice, yes. The benefit of not doing it is that it gives the flexibility to make changes to the contents of the binary distro which is useful while we're still ironing out the kinks. I'm happy to wait until someone asks for it before doing it.
Signed-off-by: Stephen Sherratt <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.