Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add new command jj config unset #4547

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Add new command jj config unset #4547

merged 2 commits into from
Oct 30, 2024

Conversation

pylbrecht
Copy link
Contributor

@pylbrecht pylbrecht commented Sep 27, 2024 •
edited
Loading

Allow unsetting config values similar to git config unset.

$ jj config set --user some-key some-val
$ jj config get some-key
some-val
$ jj config unset --user some-key
$ jj config get some-key
Config error: configuration property "some-key" not found
For help, see https://martinvonz.github.io/jj/latest/config/.

Unsetting a key, which is part of a table, might leave that table empty. For now we do not delete such empty tables, as there may be cases where an empty table is semantically meaningful (#4458 (comment)).

For example:

[table]
key = "value"

[another-table]
key = "value"

Running jj config unset --user table.key will leave us with table being empty:

[table]

[another-table]
key = "value"

Closes #4458

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch 2 times, most recently from a1925ce to 9b66e8c Compare October 5, 2024 15:00
@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch 10 times, most recently from a281997 to 910f709 Compare October 14, 2024 19:09
@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch 2 times, most recently from a7a711f to ee92399 Compare October 22, 2024 19:06
@pylbrecht pylbrecht changed the title WIP: add jj config unset Add new command jj config unset Oct 22, 2024
@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch from ee92399 to d9b0c4c Compare October 22, 2024 19:26
@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch from d9b0c4c to 239f850 Compare October 22, 2024 19:30
@pylbrecht
Copy link
Contributor Author

I would like to get some feedback on my changes. I left some open questions as inline comments.
Also, since writing Rust still feels pretty wonky, I am happy for any Rust specific feedback.

@pylbrecht pylbrecht marked this pull request as ready for review October 22, 2024 19:49
yuja
yuja reviewed Oct 23, 2024
View reviewed changes
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good to me, thanks.

cli/src/commands/config/mod.rs Outdated Show resolved Hide resolved
cli/src/commands/config/unset.rs Show resolved Hide resolved
cli/src/commands/config/unset.rs Outdated Show resolved Hide resolved
cli/src/config.rs Outdated Show resolved Hide resolved
cli/src/config.rs Outdated Show resolved Hide resolved
cli/tests/test_config_command.rs Outdated Show resolved Hide resolved
cli/tests/test_config_command.rs Outdated Show resolved Hide resolved
@pylbrecht
Copy link
Contributor Author

@yuja, thanks for your quick feedback!

I will address your comments over the next couple of days and let you know when I'm done.

@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch 3 times, most recently from c8bcdb8 to edaede3 Compare October 25, 2024 11:48
@pylbrecht pylbrecht mentioned this pull request Oct 25, 2024
Closed
@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch from edaede3 to 56f3e7b Compare October 27, 2024 15:08
@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch from 56f3e7b to 8860433 Compare October 27, 2024 16:13
yuja
yuja reviewed Oct 28, 2024
View reviewed changes
cli/src/commands/config/mod.rs Outdated Show resolved Hide resolved
cli/src/config.rs Outdated Show resolved Hide resolved
cli/src/config.rs Outdated Show resolved Hide resolved
cli/src/config.rs Outdated Show resolved Hide resolved
cli/src/config.rs Outdated Show resolved Hide resolved
@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch 2 times, most recently from ed2cd1c to b8192b8 Compare October 28, 2024 10:43
@pylbrecht
Copy link
Contributor Author

@yuja, thanks again for taking the time to look through my changes. I addressed your comments, so the changes are ready for another review. For your sake, I hope this is the last round trip. 🤞

I also added a test case for unsetting keys of inline tables, making it more explicit that we do not support this at the moment.
(When it comes to tests, I am biased towards "better too much than too little", so let me know if the overhead of another test is not worth it in this case.)

yuja
yuja approved these changes Oct 28, 2024
View reviewed changes
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

CHANGELOG.md Outdated Show resolved Hide resolved
cli/src/commands/config/unset.rs Show resolved Hide resolved
cli/src/config.rs Outdated Show resolved Hide resolved
@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch from b8192b8 to 1d7c642 Compare October 28, 2024 14:42
@pylbrecht
cli: extract read/write config helpers
a5dfd4d 
This is a preparatory refactoring.

We will introduce another function to remove config values, in which we
will reuse these two helper functions.
@pylbrecht
cli: add jj config unset
90f2c1a 
Allow unsetting config values similar to `git config unset`.

```bash
$ jj config set --user some-key some-val
$ jj config get some-key
some-val
$ jj config unset --user some-key
$ jj config get some-key
Config error: configuration property "some-key" not found
For help, see https://martinvonz.github.io/jj/latest/config/.
```

Unsetting a key, which is part of a table, might leave that table empty.
For now we do not delete such empty tables, as there may be cases where
an empty table is semantically meaningful
(jj-vcs#4458 (comment)).

For example:

```toml
[table]
key = "value"

[another-table]
key = "value"
```

Running `jj config unset --user table.key` will leave us with `table`
being empty:
```toml
[table]

[another-table]
key = "value"
```
@pylbrecht pylbrecht force-pushed the pylbrecht/push-qqylpywspyzu branch from 1d7c642 to 90f2c1a Compare October 30, 2024 05:19
@pylbrecht pylbrecht merged commit 8b5b4c1 into jj-vcs:main Oct 30, 2024
18 checks passed
@pylbrecht pylbrecht deleted the pylbrecht/push-qqylpywspyzu branch October 30, 2024 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyously joyously joyously left review comments

@yuja yuja yuja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FR: jj config unset [OPTIONS] <--user|--repo> <NAME>
3 participants
@pylbrecht @yuja @joyously