pylti / lti

Learning Tools Interoperability for Python
Other
78 stars 45 forks source link

Check full tagname when parsing xml #60

Closed cutz closed 6 years ago

cutz commented 6 years ago

Fix for #59. When parsing xml check against the full local tag name.

codecov[bot] commented 6 years ago

Codecov Report

Merging #60 into master will increase coverage by 0.04%. The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   85.47%   85.51%   +0.04%     
==========================================
  Files          16       16              
  Lines         654      656       +2     
  Branches      114      114              
==========================================
+ Hits          559      561       +2     
  Misses         75       75              
  Partials       20       20
Impacted Files Coverage Δ
src/lti/tool_config.py 90.44% <91.3%> (+0.14%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4b8fc3c...57cd62b. Read the comment docs.

cutz commented 6 years ago

In my opinion another pass at process_xml is warranted to reduce some of the duplication and simplify. It looks like a good candidate for a map of tag name -> string (attribute) or callable (for more complicated parsing). Of course that would belong in its own PR.

cutz commented 6 years ago

I'm fine inlining that if you would prefer it be done that way. I have a personal preference to wrapping it in a function but am happy to inline based on your request.

Are you worried about the overhead of function calls there or something more stylistic?

ryanhiebert commented 6 years ago

No, I just prefer to avoid abstraction when reasonable, and I don't find the wrapping function giving enough value to be worth its cost. It's a bit of a taste thing. And a bit on the principle that duplication is better than the wrong abstraction. This one seems like a less-than-ideal abstraction, and I think we'll find a better one when we get to refactoring this code, though I can't tell you exactly what I think it will be ATM. I suspect that keeping this inline will help us see it better, though.

ryanhiebert commented 6 years ago

Thanks for your work on this, it's great to have people looking at this code and working on it. It's lay dormant for a while, due to my inability to provide it with much attention.

cutz commented 6 years ago

PR updated.

Thanks for your work on this, it's great to have people looking at this code and working on it.

Sure thing, happy to help.

cutz commented 6 years ago

@ryanhiebert have you thought about when you might be able to push a new version up to pypi with these last couple of fixes?