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.

Sign up for GitHub

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

Jump to bottom

Deal with items with same name but different values in ordered field widget #76

Merged
merged 2 commits into from
Oct 3, 2018
Merged

Deal with items with same name but different values in ordered field widget #76

merged 2 commits into from
Oct 3, 2018

Conversation

rodfersou
Copy link
Contributor

fix #75

@rodfersou
Copy link
Contributor Author

@hvelarde FYI

This was referenced Jan 4, 2018
Merged
Merged
@icemac
Copy link
Member

icemac commented Feb 14, 2018

@rodfersou Thank you for your PR. Could you please add a test which assures the changed behaviour?

hvelarde
hvelarde reviewed Feb 14, 2018
View reviewed changes
src/z3c/form/browser/orderedselect.py Outdated
for item in self.items:
if not item['content'] in selecteditems:
if not item['value'] in selecteditems:
Copy link
Contributor

Choose a reason for hiding this comment

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

if item['value'] not in selecteditems:

@rodfersou
Copy link
Contributor Author

@hvelarde done
@icemac can you please point me an widget test example?

There is no test for this widget in this package.

@icemac
Copy link
Member

icemac commented Mar 12, 2018

@rodfersou The OrderedSelectWidget is tested in this doctest: z3c/form/browser/orderedselect.rst. Maybe you could use some of the code in this file to create a new unittest as the doctest also serves as documentation and should not contain too many edge case tests.

@rodfersou
Copy link
Contributor Author

@icemac I am having some trouble to finish this PR #78

@rodfersou
Copy link
Contributor Author

rodfersou commented Sep 3, 2018 •
edited
Loading

@icemac I think this change in test is sufficient, please take a look

@rodfersou
Copy link
Contributor Author

looks like I break something.. that's weird..

icemac
icemac requested changes Sep 5, 2018
View reviewed changes
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

Currently the tests break because of a syntax error. I did not yet look further into the code.

src/z3c/form/browser/orderedselect.rst Outdated
@@ -109,6 +109,7 @@ providing ``ITerms``. This source uses descriminators wich will fit our setup.
... SimpleVocabulary.createTerm(1, 'a', u'A'),
... SimpleVocabulary.createTerm(2, 'b', u'B'),
... SimpleVocabulary.createTerm(3, 'c', u'C')
Copy link
Member

Choose a reason for hiding this comment

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

This line is missing a comma, causing the test failures.

@rodfersou rodfersou force-pushed the rodfersou-orderedfield-fix branch 2 times, most recently from 675e300 to 92ce528 Compare September 5, 2018 14:43
@rodfersou
Copy link
Contributor Author

@icemac did you see my changes?

@icemac
Copy link
Member

icemac commented Sep 11, 2018

@rodfersou Did you already sign the Zope Committer Agreement? It will allow you to merge the PR on your own. (That's the policy in the zopefoundation repositories.)

@rodfersou
Copy link
Contributor Author

@icemac no, I didn't.. just the Plone Agreement

@rodfersou
Copy link
Contributor Author

@icemac signed and sent the email :-)

@rodfersou rodfersou merged commit 4524492 into zopefoundation:master Oct 3, 2018
@rodfersou rodfersou deleted the rodfersou-orderedfield-fix branch October 3, 2018 16:45
@rodfersou
Copy link
Contributor Author

@icemac a release would be appreciated

@icemac
Copy link
Member

icemac commented Oct 4, 2018

@rodfersou Currently the tests are failing on master, see https://travis-ci.org/zopefoundation/z3c.form/jobs/436732823. Are you willing to look into these failures. I think they have to do with new releases of e. g. zope.schema.

@rodfersou
Copy link
Contributor Author

@icemac wow! last month they did 6 releases at zope.schema!
can someone guide me on what is going on?

@jamadden
Copy link
Member

jamadden commented Oct 4, 2018

I looked over the test output. Everything looks cosmetic, and all related to raising more specific exceptions with different error messages than before (from both zope.schema and zope.configuration). That's great for code when you can except SomethingSpecific as ex and access attributes of ex that provide you with actionable information, but it does expose the fragility of doctests like this.

@rodfersou
Copy link
Contributor Author

image

@davisagli
Copy link
Member

I did some work on fixing the tests for the 3.x branch which can probably be easily merged to master: 4421dc4

@rodfersou
Copy link
Contributor Author

great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hvelarde hvelarde hvelarde left review comments

@icemac icemac icemac approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Not showing 2 options with same name and different values
5 participants
@rodfersou @icemac @jamadden @davisagli @hvelarde