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

Recursive property query #106

Closed
wants to merge 4 commits into from
Closed

Recursive property query #106

wants to merge 4 commits into from

Conversation

jforand
Copy link
Contributor

@jforand jforand commented Apr 23, 2018

This would allow for getting a single property, rather than the entire list, as well as being able to recursively get a property.

@coveralls
Copy link

coveralls commented Apr 23, 2018

Coverage Status

Coverage decreased (-3.005%) to 68.464% when pulling 4e62299 on jforand:master into 95922a3 on dsoprea:master.

@jforand-adacel
Copy link

I'm not super thrilled with this change tbh.

Mailnly, I don't like the multiple return types, but I also wonder if it wouldn't be better to just change the method signature on properties() to be able to query a single property, and to be able to do it recursively. In the case of recursively, you would still run into the issue of the multiple return types (Either a dict{str: str} or a dict{str: list(str: str)}.

@dsoprea
Copy link
Owner

dsoprea commented Apr 24, 2018

Do me a favor and squash. Then, I'll take a look. It'll be nicer to review and we won't be introducing a series of partial commits.

@dsoprea
Copy link
Owner

dsoprea commented Apr 24, 2018

I'll be happen to see if there's a way to mitigate your concerns (thanks for being honest :) ). You'll also have to write some unit-tests. Coverage dropped quite a bit due to your changes.

@jforand
Copy link
Contributor Author

jforand commented Apr 24, 2018

I'm not 100% sure how to do that properly, so I'll close this request, and re-open a new one, with tests =D.

@jforand jforand closed this Apr 24, 2018
@dsoprea
Copy link
Owner

dsoprea commented Apr 25, 2018 via email

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.

4 participants