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

Feature: Support yaml list as workspace root. #5156

Merged

Conversation

@birgerbr
Copy link
Contributor

@birgerbr birgerbr commented May 16, 2019

Changelog: Feature: Support yaml lists in workspace root field.
Docs: conan-io/docs#1288

This pull-request closes #5155

Using yaml list for setting the workspace root variable makes it easier
to manage the yaml file, when the workspace has multiple roots.
@CLAassistant
Copy link

@CLAassistant CLAassistant commented May 16, 2019

CLA assistant check
All committers have signed the CLA.

@jgsogo
Copy link
Member

@jgsogo jgsogo commented May 17, 2019

Hi, it looks pretty good. Can you please add a test like this one using a yaml list? (maybe you can just parameterize that unittest).

If you need help with the test, please let me know.

Thanks for the contribution!

@birgerbr
Copy link
Contributor Author

@birgerbr birgerbr commented May 20, 2019

Hi, it looks pretty good. Can you please add a test like this one using a yaml list? (maybe you can just parameterize that unittest).

If you need help with the test, please let me know.

Thanks for the contribution!

Thanks! I've added a new test based on multiple_roots_test. Using parametrize would reduce the code duplication. Is it parameterized.parameterized (from parameterized import parameterized) that should be used then?

@jgsogo
Copy link
Member

@jgsogo jgsogo commented May 20, 2019

Yes, we use parameterized.parameterized, you can find it at several places in the test suite.

@birgerbr
Copy link
Contributor Author

@birgerbr birgerbr commented May 20, 2019

Should I create a new commit which uses parameterized.parameterized instead of having a separate test?

@jgsogo
Copy link
Member

@jgsogo jgsogo commented May 20, 2019

Yes, I think it is much better to avoid duplicating code. Those test should behave exactly the same.

The `root_as_yaml_list_test` test is removed and the
`multiple_roots_test` is parametrized to test for the different root
attribute formats.
@birgerbr
Copy link
Contributor Author

@birgerbr birgerbr commented May 20, 2019

I've refactored the test to parametrize multiple_roots_test instead of having an additional test.
Another question: Should I squash these commits into a single commit?

@lasote lasote requested a review from memsharded May 20, 2019
@lasote lasote assigned memsharded and unassigned jgsogo May 20, 2019
@lasote lasote added this to the 1.16 milestone May 20, 2019
@jgsogo
Copy link
Member

@jgsogo jgsogo commented May 20, 2019

Don't worry about it, we squash on merge. Thanks a lot!

@birgerbr
Copy link
Contributor Author

@birgerbr birgerbr commented May 20, 2019

Thanks 👍 Should be ready for review now.

jgsogo
jgsogo approved these changes May 20, 2019
@memsharded
Copy link
Member

@memsharded memsharded commented May 22, 2019

Very nice, and also good test, thanks @birgerbr for the contribution and @jgsogo for the collaboration, good job.

@memsharded memsharded merged commit a64c3f4 into conan-io:develop May 22, 2019
2 checks passed
@memsharded
Copy link
Member

@memsharded memsharded commented May 22, 2019

Will be released in Conan 1.16

@memsharded
Copy link
Member

@memsharded memsharded commented May 22, 2019

@birgerbr

Note how I have updated the original first comment to add for Changelog and docs. This is automatically used in our releases for the changelog. Also please have a look to the PR of the contributed doc, and check if it is correct, please. Every PR that changes or add things for users, should have an accompanying PR to the docs.

Many thanks again!

@birgerbr
Copy link
Contributor Author

@birgerbr birgerbr commented May 23, 2019

Thanks! I'll remember that for the next time.
I've looked through the doc changes. They look good to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants