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.

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

workspace-update-stale: add a config knob to run automatically #4807

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

torquestomp
Copy link
Collaborator

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

@torquestomp
Copy link
Collaborator Author

Fixes issue #3820

@PhilipMetzger
Copy link
Collaborator

In my opinion the PR contains three unrelated topics in the stack. As the first commit is a docs cleanup (1), the second two are moving the recovering commit logic into lib (2) (which makes it usable for the Google server) and then the actually advertised PR (3) which depends on (2). I think at least the first commit should be split out, since it has no relation to rest, the other two would be nice to have but can stay together.

And all commits are missing a motivation, as it definitely is not obvious what they want to achieve. Quoted from contributing.md:

We are not particularly strict about the style, but please do explain the reason for the change unless it's obvious.

@torquestomp
Copy link
Collaborator Author

In my opinion the PR contains three unrelated topics in the stack. As the first commit is a docs cleanup (1), the second two are moving the recovering commit logic into lib (2) (which makes it usable for the Google server) and then the actually advertised PR (3) which depends on (2). I think at least the first commit should be split out, since it has no relation to rest, the other two would be nice to have but can stay together.

The only difference between (2) and (3) is one moves stuff to lib, the other to cli_util, I don't think they're very different. I split (1) to its own PR but it's still here too, I'm not sure how to set up dependent PRs with jj.

And all commits are missing a motivation, as it definitely is not obvious what they want to achieve. Quoted from contributing.md:

We are not particularly strict about the style, but please do explain the reason for the change unless it's obvious.

Added some more text to the commit descriptions.

lib/src/workspace.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
lib/src/working_copy.rs Outdated Show resolved Hide resolved
lib/src/working_copy.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Show resolved Hide resolved
Comment on lines +913 to +951
impl<E> From<E> for SnapshotWorkingCopyError
where
E: Into<CommandError>,
{
fn from(value: E) -> Self {
Self::Command(value.into())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I wouldn't add automatic conversion because the caller should be careful to not map stale wc error to Command(_).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Making this explicit makes snapshot_working_copy() pretty terrible; there are far more existing implicit conversions into CommandError than there are places that need to return an explicit StaleWorkingCopy error (10+ > 3)

cli/src/cli_util.rs Show resolved Hide resolved
This is to facilitate automatic update-stale in extensions and in the CommandHelper layer.
This is to facilitate automatic update-stale in extensions and in the CommandHelper layer.
This centralizes the logic so the cli helpers can use it.
This centralizes the logic so cli helpers can use it.
This significantly reduces toil for multi-workspace users, resolving issue martinvonz#3820
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants