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

Added lxml.html.CheckboxGroup.value_options property #57

Merged
merged 2 commits into from
Jun 26, 2012

Conversation

alxistr
Copy link
Contributor

@alxistr alxistr commented Jun 22, 2012

In lxml.html.CheckboxGroup.doc exists description for property .value_options that is not implementated. Added method is the same as in RadioGroup.

@scoder
Copy link
Member

scoder commented Jun 26, 2012

This is lacking tests.

@alxistr
Copy link
Contributor Author

alxistr commented Jun 26, 2012

This commit append 7 lines that is not influence for something. Moreover, this code should be there for a long time, since it is documented. I'll search similar tests for RadioGroup, and if they are exists I'll add them.

@scoder
Copy link
Member

scoder commented Jun 26, 2012

I'm inclined to think that the documentation was just a copy&paste oversight. And I do not see the relation between (the potential lack of) tests for RadioGroup and any tests for CheckgroupBox.

But I do agree that this would be helpful, so I'm asking you to finish up your changes (which obviously includes tests) so that I can merge them.

@alxistr
Copy link
Contributor Author

alxistr commented Jun 26, 2012

In tests folder there is no any tests for forms and controls. Should I add this too? Even there is no html file with forms and controls, only one with simple divs (lxml/tests/shakespeare.html).

@alxistr
Copy link
Contributor Author

alxistr commented Jun 26, 2012

I mean, If I add the tests for it commit that it will have to make too great additions like append html file with

etc. Because there is no other similar tests.

@scoder
Copy link
Member

scoder commented Jun 26, 2012

The tests for lxml.html are in

https://github.com/lxml/lxml/tree/master/src/lxml/html/tests/

Most of them use doctests.

@alxistr
Copy link
Contributor Author

alxistr commented Jun 26, 2012

I think it is finished. Is it fine that commits have same names?

@scoder
Copy link
Member

scoder commented Jun 26, 2012

I prefer either having tests and feature in the same commit (if they are reasonably small) or giving each separate commit a telling name, e.g. "added tests for new .value_group property" or something like that for the commit with the tests. However, given that this would best have been one commit, I'm fine with having one commit comment for both.

Thanks for the contribution.

scoder added a commit that referenced this pull request Jun 26, 2012
Added lxml.html.CheckboxGroup.value_options property
@scoder scoder merged commit 57d2b5a into lxml:master Jun 26, 2012
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.

2 participants