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

fix: Fix issue with update subdirectory given an answers file path #452

Conversation

bburgin
Copy link
Contributor

@bburgin bburgin commented Sep 25, 2021

Fixes #451

Justification

During update, the destination _subdirectory is joined with the rendered answers_file value. We want to avoid including the same folders twice in the path.

Implementation

Write the answers filename to the config, but without the path.

Testing

Added unit test for this case.

@yajo
Copy link
Member

yajo commented Sep 26, 2021

This one doesn't seem to fix the linked issue.

Keep in mind that the subdirectory option belongs to the template, and indicates what subdirectory of the template should I render in dst. Whereas the answers_file option belongs to the destination and indicates where the answers file is located inside dst. I think you're mixing subjects.

The expected fix is to allow updates in destination when destination is inside a git repo, no matter if it's the root of the repo or not.

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2021

Codecov Report

Merging #452 (44e6cbf) into master (b8ff4d8) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #452      +/-   ##
==========================================
+ Coverage   96.32%   96.36%   +0.04%     
==========================================
  Files          39       39              
  Lines        2473     2504      +31     
==========================================
+ Hits         2382     2413      +31     
  Misses         91       91              
Flag Coverage Δ
unittests 96.36% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
copier/main.py 94.55% <100.00%> (+0.01%) ⬆️
tests/test_subdirectory.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8ff4d8...44e6cbf. Read the comment docs.

@bburgin
Copy link
Contributor Author

bburgin commented Sep 26, 2021

@yajo:

Thank you for the clarifications. I am still trying to understand the various use cases and what we can do in the code to meet them. I see two use cases:

  • subdirectory use case 1: The user goes to the subdirectory of a repo and runs update from there. The subdirectory contains the answer file.
  • subdirectory use case 2: The user goes to the root of the repo and runs update from there, without cd'ing into the subdirectory that contains the answer file.

I think we should support both use cases. Otherwise, how would the user know which is the correct usage?

The expected fix is to allow updates in destination when destination is inside a git repo, no matter if it's the root of the repo or not.

I think you are correct. The issue (subdirectory use case 1) that I linked in this PR may be the wrong issue for the changes I made. I meant my changes to fix subdirectory use case 2.

For subdirectory use case 2
Without the fix from this PR, the update call fails. The user gets doubled paths for the answer file and an error that the file could not be found. See the unit test in this PR for an example of the use case and inputs to copier. You can see the error by running the unit test but without the fix I made in main.py.

Potential action items
I could remove the mention of the issue (subdirectory use case 1) from this PR and proceed forward with it to fix subdirectory use case 2. What do you think?

Keep in mind that the subdirectory option belongs to the template, and indicates what subdirectory of the template should I render in dst. Whereas the answers_file option belongs to the destination and indicates where the answers file is located inside dst. I think you're mixing subjects.

I am open to suggestions on how to improve the changes in this PR, to still fix subdirectory use case 2. What changes do you want me to make, so that it better aligns with the copier design and code?

Thank you.

@yajo
Copy link
Member

yajo commented Sep 27, 2021

subdirectory use case 2: The user goes to the root of the repo and runs update from there, without cd'ing into the subdirectory that contains the answer file.

Just forget about this use case. Once you fix case 1, this will work automatically with:

copier update path/to/subfolder

We can't guess where the subfolder is because a single project can have more than 1 subfolders that each one use different templates.

The copier-answers path provided with -a is always relative to the root of the destination (path/to/subfolder in above example).

@bburgin
Copy link
Contributor Author

bburgin commented Sep 30, 2021

Thanks. Are you saying that subdirectory use case 2 should not be supported?

I think it could be supported with this PR change that I have made. The user can provide enough information in the -a argument so that the path down to the answers file is not ambiguous. The -a argument can contain the relative path folders down to the answer file from the destination path, as I have shown in the unit test.

If this is still not acceptable, then can we make copier return a better error for this case? Something like:
"This case is not supported. Please always pass folder information via the destination path not the -a argument. The -a argument must be a filename only, not a path."

@bburgin
Copy link
Contributor Author

bburgin commented Sep 30, 2021

I am currently working on the subdirectory use case 1 and will post a separate PR for it.

@yajo
Copy link
Member

yajo commented Oct 1, 2021

Yes, case 2 is conceptually wrong, because -a is not meant to explain where the root of your project is. Instead it just tells where to find the answers, and those could perfectly exist in a subdirectory.

The thing that tells the project root is dst_path.

If your project has several subsections that apply templates, you should indicate that in dst_path. If a subsection uses more than 1 template, then you'll need different -a inside that subsection.

I am currently working on the subdirectory use case 1 and will post a separate PR for it.

Ok, closing this one as explained. Thanks!

@yajo yajo closed this Oct 1, 2021
@yajo yajo added this to the v6.0.0 milestone Mar 7, 2022
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.

Support updating a subproject when it isn't in the root of the git repo
3 participants