fedora-hubs

#324 Improve the widget configuration validators
Merged by abompard. Opened by abompard.
abompard/fedora-hubs widget-config  into  develop

Download 324.patch

Use the validators's return value for the widget conf

The validators were returning a converted value (for example, to integer) but that value was ignored and the widgets had to do the conversion again themselves when they wanted to use that value.

This changeset make use of the validators to convert to and from the string that was passed to the config form. It also moves them to the widgets module because validators are actually only used by widgets, so it makes more sense to have them with the other widget-related modules.

There also two other smaller fixes. As usual, I tried to keep the commits topical.

Perhaps add an assertion that this result is a 200.

A 200 assertion would be good here too.

I would recommend tracking TODOs in the issue tracker rather than in code. In my experience, code TODOs often sit for a long time and become forgotten.

Same here - I recommend filing this as an issue rather than putting a TODO.

Here as well.

And here.

I recommend adding docblocks on all the classes and methods in this file, so that all the types are clearly indicated and the purposes of the classes/methods are documented.

No blockers, just some suggestions that you can apply at your option. I also recommend writing tests for this change, as it appears that the test suite isn't altered by these commits.

Thanks for your review ! I just converted the existing functions but you're right, it's the perfect time to add documentation :)

I'll also add tickets for the unimplemented validators, but I'll leave the TODO comments too, for those who just happen on the code and might want to fix it ;-)

Oh, and I've added unit tests too. Writing unit tests for validators is so easy that it would be a shame to miss it ;-)
Edited by abompard

3 new commits added

  • Add unittests for validators
  • Add two more assertions in the tests
  • Add and fix documentation

2 new commits added

  • Provide a sensible default for the Username validator
  • Cleanup blank spaces

LGTM

Pull-Request has been merged by abompard
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.