Opened 9 years ago

Closed 9 years ago

#284 closed defect (fixed)

Not every 'data' parsing is done with json

Reported by: jelte Owned by: jelte
Priority: low Milestone: 06. 4th Incremental Release
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?: no

Description

Some code still uses ast.literal_eval, which happens to have mostly the same properties, but not completely, should change those to json (and to be sure, make prettyprints also in correct json)

Subtickets

Change History (6)

comment:1 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from new to reviewing

fix + lots of tiny data representation fixlets (json parser wants true/false instead of True/False?, and no comma at the end of lists and maps) in branches/trac284 r2441

comment:2 in reply to: ↑ 1 ; follow-up: Changed 9 years ago by jinmei

  • billable set to 1
  • Internal? unset

Replying to jelte:

fix + lots of tiny data representation fixlets (json parser wants true/false instead of True/False?, and no comma at the end of lists and maps) in branches/trac284 r2441

  • general: I'm not sure why you changed single quote to double-quote (although I have no objection to that). Is it just some style change or something inevitable for this ticket?
  • general: I don't know if s/false/False/ is necessary, but I trust you on this point:-)
  • same for s/ast.literal_eval/json.loads/ (about this is correct)
  • cmdctl_test.py: isn't it better to imprt sys at the beginning of the file?
  • cfgmgr.py: I didn't understand why pprints were commented out. If they are not necessary anymore why not just clean them up?
  • config/module_spec.py
    • better to import at the beginning of the file?
    • not necessarily for this changeset, but I personally see read(-1) rather confusing. Wouldn't it be clearer to pass nothing to read()? Or at the risk of looking redundant we might define a constant such as READ_EVERYTHING = -1 and pass it to read()
    • why aren't exceptions in the 'if' case caught? Also, is it okay/safe to ignore the error and blindly initialize module_spec with an empty config? (shouldn't we rather fail with reporting an error?)
    • is it a good idea to catch all exceptions here? I wouldn't necessarily object to all catch-all catches, but in general we should be careful about that, and in (rare) cases it's justified I'd like to see an explicit comment about why we need catch everything here and why it's justified.
    • we probably need test cases when json.loads fails.

comment:3 Changed 9 years ago by jinmei

  • Milestone set to 06. 4th Incremental Release
  • Owner changed from UnAssigned to jelte

comment:4 in reply to: ↑ 2 ; follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei
  • general: I'm not sure why you changed single quote to double-quote (although I have no objection to that). Is it just some style change or something inevitable for this ticket?
  • general: I don't know if s/false/False/ is necessary, but I trust you on this point:-)

Both of these are also because the python json parser is pretty strict in this regard, it does not accept single quotes, nor False and True.

  • same for s/ast.literal_eval/json.loads/ (about this is correct)

Well, that's for consistency, literal_eval parses python structures, json.loads parses JSON. They look a lot alike but are subtly different (as you've found out in your first 2 comments :) ). And since we 'officially' use JSON for this, even though our c++ json parser can handle 'python representation' (i.e. single quotes, False, etc.), for consistency it's better to use the JSON parser.

  • cmdctl_test.py: isn't it better to imprt sys at the beginning of the file?
  • cfgmgr.py: I didn't understand why pprints were commented out. If they are not necessary anymore why not just clean them up?
  • config/module_spec.py
    • better to import at the beginning of the file?

all three ack, updating

  • not necessarily for this changeset, but I personally see read(-1) rather confusing. Wouldn't it be clearer to pass nothing to read()? Or at the risk of looking redundant we might define a constant such as READ_EVERYTHING = -1 and pass it to read()

oh, yes, no argument is ok too, and more clear

  • why aren't exceptions in the 'if' case caught? Also, is it okay/safe to ignore the error and blindly initialize module_spec with an empty config? (shouldn't we rather fail with reporting an error?)

technically this case is still caught (it'll throw an exception later if it's empty) but the error then is misleading. Catching and throwing right away.

  • is it a good idea to catch all exceptions here? I wouldn't necessarily object to all catch-all catches, but in general we should be careful about that, and in (rare) cases it's justified I'd like to see an explicit comment about why we need catch everything here and why it's justified.

No, that's an oversight (initially there because the documentation isn't clear on what exceptions can be thrown). Changing to the right one.

  • we probably need test cases when json.loads fails.

ack

Updates done in r2532, please verify.

comment:5 in reply to: ↑ 4 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

Replying to jelte:

  • not necessarily for this changeset, but I personally see read(-1) rather confusing. Wouldn't it be clearer to pass nothing to read()? Or at the risk of looking redundant we might define a constant such as READ_EVERYTHING = -1 and pass it to read()

oh, yes, no argument is ok too, and more clear

There's one more occurrence of read(-1) in r2532. I removed it and
committed the change to the branch (r2534).

Other changes look okay.

comment:6 Changed 9 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Ok, merged in r2541, closing ticket.

Thanks for the review and update!

Note: See TracTickets for help on using tickets.