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

feat: add support for yaml data files #1059

Closed
wants to merge 17 commits into from
Closed

feat: add support for yaml data files #1059

wants to merge 17 commits into from

Conversation

Ian2012
Copy link

@Ian2012 Ian2012 commented Mar 27, 2023

This PR adds support for yaml files as a data input

@sisp
Copy link
Member

sisp commented Mar 28, 2023

Follow-up of #485. Resolves #540.

Copy link
Member

@sisp sisp left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, @Ian2012!

I have a couple of minor suggestions and raised a topic for discussion around repeatedly passing the -d, --data switch with data files.

Please also add tests for both the success and failure scenarios once the conceptual discussion has been resolved.

As always, @yajo needs to perform the ultimate review, I'm only trying to support.

copier/cli.py Outdated Show resolved Hide resolved
copier/cli.py Show resolved Hide resolved
copier/cli.py Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
@Ian2012
Copy link
Author

Ian2012 commented Mar 28, 2023

@sisp Thanks for your review. I've already handle all your comments, if you agree, could you resolve all the conversations so we can focus on the conceptual discussion?

@sisp
Copy link
Member

sisp commented Mar 29, 2023

Thanks, @Ian2012. There are still a few open suggestions of mine in the docs though. And I can't seem to mark the other comments as resolved. Can you resolve them?

@Ian2012
Copy link
Author

Ian2012 commented Apr 4, 2023

Is there anything I can do to speed up the review process?

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #1059 (30afa27) into master (17135c5) will decrease coverage by 0.40%.
The diff coverage is 40.00%.

❗ Current head 30afa27 differs from pull request most recent head 5c20f0c. Consider uploading reports for the commit 5c20f0c to get more accurate results

@@            Coverage Diff             @@
##           master    #1059      +/-   ##
==========================================
- Coverage   96.49%   96.09%   -0.40%     
==========================================
  Files          44       43       -1     
  Lines        3394     3203     -191     
==========================================
- Hits         3275     3078     -197     
- Misses        119      125       +6     
Flag Coverage Δ
unittests 96.09% <40.00%> (-0.40%) ⬇️

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

Impacted Files Coverage Δ
copier/cli.py 91.01% <40.00%> (-6.52%) ⬇️

... and 10 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

docs/configuring.md Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
@sisp
Copy link
Member

sisp commented Apr 5, 2023

@Ian2012 I think it would be good to mention in the docs for both --data and --data-file (with anchor links to each other) that they are mutually exclusive.

@Ian2012
Copy link
Author

Ian2012 commented Apr 5, 2023

I totally agree, here is an updated version https://copier--1059.org.readthedocs.build/en/1059/configuring/#data

docs/configuring.md Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Some rewording on docs and simplification on code.

Thanks for the feature! Really helpful and a small patch. I like it a lot 😊

This needs a test though. Could you add it too please?

copier/cli.py Outdated Show resolved Hide resolved
copier/cli.py Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
Ian2012 and others added 6 commits April 6, 2023 08:57
Co-authored-by: Jairo Llopis <973709+yajo@users.noreply.github.com>
Co-authored-by: Jairo Llopis <973709+yajo@users.noreply.github.com>
Co-authored-by: Jairo Llopis <973709+yajo@users.noreply.github.com>
Co-authored-by: Sigurd Spieckermann <2206639+sisp@users.noreply.github.com>
Co-authored-by: Sigurd Spieckermann <2206639+sisp@users.noreply.github.com>
Co-authored-by: Jairo Llopis <973709+yajo@users.noreply.github.com>
- CLI flags: `--data-file`
- Default value: N/A

As an alternative to [`--data`][data] you can also pass the path to a YAML file that contains your data.
Copy link
Member

@sisp sisp Apr 6, 2023

Choose a reason for hiding this comment

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

A newline is missing. Also, how about adding the short flag here as well? And shouldn't the link be an anchor link?

Suggested change
As an alternative to [`--data`][data] you can also pass the path to a YAML file that contains your data.
As an alternative to [`-d, --data`](#data) you can also pass the path to a YAML file that contains your data.

- Default value: N/A

As an alternative to [`--data`][data] you can also pass the path to a YAML file that contains your data.
!!! info
Copy link
Member

Choose a reason for hiding this comment

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

A newline is missing:

Suggested change
!!! info
!!! info

@Ian2012
Copy link
Author

Ian2012 commented Apr 6, 2023

I would like to add tests but I'm unable to setup a development environment using local env, do you guys have the manual instructions?

When I run direnv allow it does nothing, I don't know how to install poe and the developments dependencies

@yajo
Copy link
Member

yajo commented Apr 6, 2023

There's a small guide in https://github.com/copier-org/copier/blob/master/CONTRIBUTING.md#dev-environment-setup

The fastest way is just to use gitpod.

@Ian2012
Copy link
Author

Ian2012 commented Apr 10, 2023

@yajo I'm really struggling with the test, could you give me a hand there?

I'm not used to test

@sisp
Copy link
Member

sisp commented Apr 12, 2023

@Ian2012 If you're still struggling with setting up the development environment, it's typically sufficient to run

$ poetry install --with dev,docs
$ poetry run pip install black isort  # optionally but useful to get the formatters
$ poetry run poe test

if you have Git and a sufficiently recent Python interpreter installed on your system. Nix provides a complete and deterministic setup including Git, Python, pre-commit, code formatters and linters, but you should be able to develop also without it.

@yajo
Copy link
Member

yajo commented Apr 19, 2023

I'm not used to test

I think it's more a matter of not knowing how to do a unit test, right?

I'll try to help when I have some free time. Or maybe you can help with that @sisp?

@yajo
Copy link
Member

yajo commented Sep 19, 2023

Superseded by #1325.

@yajo yajo closed this Sep 19, 2023
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