Code review comment for lp:~rharding/charmworld/proof-config
Revision history for this message
| Gary Poster (gary) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
67d34a1
(Get the code!)
Hi Rick. Thank you for a nice branch. LGTM.
I give a ton of comments that could lead to a decent amount of work.
Please treat all of them as suggestions, though I do think that the code
in the BundleProof code ought to move back to the view, somehow, and I
hope I made at least a somewhat compelling case for it.
https:/
File charmworld/
https:/
charmworld/
On 2013/10/18 19:09:24, rharding wrote:
> I'm not a fan of importing models into a lib file, open to ideas of
ways to make
> this better other than passing in a resolver and a descriptor into the
proof
> calls from the view.
Similar alternative: You have to instantiate BundleProof with these
extra bits, to create something that the view can use? Don't know
charmworld, so I don't know where it would be stashed, but that broad
approach seems tried and true.
https:/
charmworld/
format='yaml'):
Ah, ye olde naming problem: a validation function also does work. :-)
If you really cared about the naming problem, I would suggest naming
this "parse_
tossing out any helpful information from the parser's error, btw :-/)
seems minor compared to the fact that this parses the data. <shrug>
(which means "do as you will")
https:/
charmworld/
Uh oh, this validation function is doing work too. :-/
Um.
This doesn't feel right. The Bundle proof bits here feel like they
belong in an integration layer, like a view, rather than in a library.
Do with that what you will.
https:/
charmworld/
On 2013/10/18 19:09:24, rharding wrote:
> this is only called if it's expecting to proof options. So not having
any is an
> error. (test_key and test_value are required inputs)
Is this an error of the bundle author or in the charmworld code? It
feels like the latter, in which case raising a ProofError seems wrong.
We ought to log something instead. If I'm wrong, nevermind.
https:/
File charmworld/
https:/
charmworld/
...This is JSON...
https:/
charmworld/
parseable as yaml."""
"...as YAML or JSON."?
https:/
File charmworld/
https:/
charmworld/
I think this method is getting too big. Could we divide it up into
logical chunks, so one can read what's going on in the top level, and
then dig down as necessary?
https:/
charmworld/
On 2013/10/18 19:36:14, rharding wrote:
> this is the list of actual errors. They can be global messages ('could
not parse
> the file') or objects from error checking a specific bundle in the
file.
Add as comment?
https:/
charmworld/
On 2013/10/18 19:36:14, rharding wrote:
> this is a summary of all error messages provided as an aid to marco.
s/marco/proof/ and add as comment?
https:/
charmworld/
On 2013/10/18 19:36:14, rharding wrote:
> and a global level 'debug_info' to match the representation of errors
of other
> areas of the code where they all provide a message as well as helpful
debug
> information to assist in finding the way to correct the error.
Add as comment? :-)
https:/
charmworld/
On 2013/10/18 19:36:14, rharding wrote:
> moved the old checks to the try:catch for the ProofError. I felt this
read a bit
> cleaner and allows for a function to return a value (the parsed yaml
file)
> normally, but have a complex object come back during a failure.
Ah, I see. I essentially was suggesting the old approach.
Because of the concerns you and I both raised, I personally lean towards
believing that keeping it here in the view code--albeit perhaps in a
separate function, for readability, as you said--is a better approach.
https:/
charmworld/
Per my suggestion to divide this up a bit, could this be one method?
https:/
charmworld/
BundleProof.
As before: in this case I think keeping the code here in the view
module--in a separate function, perhaps--might be a better choice.
https:/
charmworld/
the charm we found.
Per my suggestion to divide this up a bit, could this be another method?
https:/
File charmworld/
https:/
charmworld/
test_bundle_
If you divide up the big validation method, you could test smaller
chunks too. I like these tests, though.
https:/