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

Add force parameter to create #1870

Closed
wants to merge 8 commits into from

Conversation

svdgoor
Copy link
Contributor

@svdgoor svdgoor commented Jun 9, 2024

Adds a parameter to the briefcase create command to skip confirming overwriting existing scaffolds. This makes it easier to test this part of building, such as when debugging new changes to this system, or to investigate problems when people (like myself earlier today) have an issue in their scaffold.

The tests do not pass anymore. I looked at them, and I see why, but I would rather leave the decision about how to solve them to you. I hope CI will automatically apply to the PR, otherwise I will paste a log of vox here later.

@mhsmith
Copy link
Member

mhsmith commented Jun 9, 2024

The tests do not pass anymore. I looked at them, and I see why, but I would rather leave the decision about how to solve them to you. I hope CI will automatically apply to the PR, otherwise I will paste a log of vox here later.

CI isn't running the tests yet because there are failures in the pre-commit and towncrier checks. To find out how to fix these, follow the "Details" links below.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The general approach you've taken here makes sense; however:

  1. You're inconsistent in the use of force, auto_overwrite and auto_overwrite as a description for this feature in the code. Pick one representation, and stick to it.
  2. Passing tests isn't an "optional, you can fix that for me" part of the contribution process. If you want this PR to be merged, you need to provide a PR that passes tests. We're happy to provide guidance on what tests are required if it's unclear - but in this case, you haven't provided any tests at all, and the existing tests are failing.

@svdgoor
Copy link
Contributor Author

svdgoor commented Jun 21, 2024

Apologies, my changes were not yet complete. I am still working on them.

@svdgoor
Copy link
Contributor Author

svdgoor commented Jun 21, 2024

perhaps, @freakboy3742, you could provide guidance on how to adapt the tests so that they still capture the way empty (parameter-less) commands have their help generated. Currently, the testing units is using the create method (which is empty before this change) as an empty command for its unit tests.

@svdgoor
Copy link
Contributor Author

svdgoor commented Jun 21, 2024

Update:

  • Resolved failed test cases where it makes sense; in the tests specifically for the create command
  • Added TODOs for each of the test cases which failed on Windows, where create was merely used as a base command.

The tests still fail since the create command was used as placeholder.

This is something I believe should be addressed as follows:

  1. Create a placeholder command
  2. Make this command available only when running unit tests
  3. Run the unit tests against this command

@svdgoor svdgoor changed the title Add auto overwrite parameter to create Add force parameter to create Jun 21, 2024
@freakboy3742
Copy link
Member

Update:

  • Resolved failed test cases where it makes sense; in the tests specifically for the create command
  • Added TODOs for each of the test cases which failed on Windows, where create was merely used as a base command.

The tests still fail since the create command was used as placeholder.

This is something I believe should be addressed as follows:

  1. Create a placeholder command
  2. Make this command available only when running unit tests
  3. Run the unit tests against this command

There's another option: promote --force to a top level option. We already have --no-input; most commands don't accept input, so the option clearly isn't at the base level because it's "universal". --force is a sufficiently common "YOLO" option that I'd be comfortable adding it as a top-level option in BaseCommand so that any command could potentially honour (although, at present, it would only be used by create).

Unless I'm mistaken, this should simplify most of the test cases you've identified as a problem.

@svdgoor
Copy link
Contributor Author

svdgoor commented Sep 20, 2024

I'm not going to finish this. If someone wants to pick it up the code is still here.

@svdgoor svdgoor closed this Sep 20, 2024
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