ImperialCollegeLondon / R2T2

Research References Tracking Tool
MIT License
14 stars 155 forks source link

Set up project to use Poetry for build, dependencies and deployment #67

Closed jezcope closed 4 years ago

jezcope commented 4 years ago

Now ready for review. ~Not ready to merge yet, but opening the PR means I can check the CI still works while I update the documentation.~

codecov[bot] commented 4 years ago

Codecov Report

Merging #67 into develop will increase coverage by 25.96%. The diff coverage is 93.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop      #67       +/-   ##
============================================
+ Coverage    65.08%   91.05%   +25.96%     
============================================
  Files            5        8        +3     
  Lines          169      369      +200     
============================================
+ Hits           110      336      +226     
+ Misses          59       33       -26     
Impacted Files Coverage Δ
r2t2/writers.py 59.25% <ø> (+3.70%) :arrow_up:
r2t2/static_parser.py 89.65% <77.77%> (-10.35%) :arrow_down:
r2t2/__main__.py 89.02% <88.15%> (+89.02%) :arrow_up:
r2t2/core.py 96.00% <96.29%> (-0.30%) :arrow_down:
r2t2/docstring_parser.py 100.00% <100.00%> (ø)
r2t2/docstring_reference_parser.py 100.00% <100.00%> (ø)
r2t2/plain_text_parser.py 100.00% <100.00%> (ø)
r2t2/runtime_tracker.py 80.00% <100.00%> (+80.00%) :arrow_up:
... and 3 more

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 d8be69b...7cedda1. Read the comment docs.

jezcope commented 4 years ago

Ok, this seems to be working now so I'm converting it to a full PR for review.

jezcope commented 4 years ago

Hunh. So it looks like maybe mypy wasn't being run before on CI, so some type inconsistencies have slipped through the net.

jezcope commented 4 years ago

Ok, it's passing CI now, but @ChasNelson1990 please check that last commit carefully because I had to make a few changes to your code to make mypy happy with it.

ChasNelson1990 commented 4 years ago

Hey @jezcope did you mean me or @de-code who (I think) wrote the docstring codes?

jezcope commented 4 years ago

Sorry yes, actually it's @billbrod's code that I had to modify, though @de-code wrote the original. git blame for the win!

billbrod commented 4 years ago

Just checked it out -- with these changes, the DOIs parsed from notebook markdown cells will always have line: 0 (in my PR, it had been line: n/a). I had originally put it as n/a after discussing it with @tinosulzer, and we agreed that it doesn't really make sense to have line number from a markdown cell -- it doesn't really have one, and saying line: 0 was a bit confusing (here, I'm pretty sure it happens because CodeDocumentComment.lineno = None, and when creating the FunctionReference object from it in function get_function_reference_from_docstring, it initializes with line=docstring.lineno or 0 -- that part of the function wasn't changed in this PR so I can't comment directly on it to flag its location).

I agree that using None is better than a weird special-case str like n/a, but I think that Line: 0 in the user-facing output is a little confusing for a markdown cell.

Otherwise, everything in that commit looks fine to me (I'm not familiar with mypy, so don't understand what changes were required for that).

jezcope commented 4 years ago

Thanks @billbrod, that's exactly the info I needed to understand the rationale here: I figured I'd probably not have got it quite right first attempt!

To summarise the type checking issue here: CodeDocumentComment.lineno is annotated to be Optional[int], meaning it can be an int or None but not a str, so setting it to "n/a" caused mypy to complain. I changed the internal "n/a" to None as you saw, and then had to fix up a few other places that impacted. The other option was to let it be Optional[Union[int, str]] (i.e. an integer or a string or nothing) which was just messy!

I agree with your point about wanting to use "n/a" for the user-facing message though, so I'll work that in.

billbrod commented 4 years ago

Ah gotcha, that all makes sense to me! I agree that changing the annotation to Optional[Union[int, str]] is messy (and we don't want it to be any str, just 'n/a'), so maybe something like: allow FunctionReference.line to be None and then convert it to "n/a" when converting it to output?

jezcope commented 4 years ago

Like this? :point_up:

de-code commented 4 years ago

BTW I am also in favour for setting values to None. The only reason that I had set it to a different value in FunctionReference was because it wasn't marked as optional and I just wanted to appease the linting. Sorry.

jezcope commented 4 years ago

@all-contributors please add @billbrod for review

allcontributors[bot] commented 4 years ago

@jezcope

I've put up a pull request to add @billbrod! :tada:

jezcope commented 3 years ago

Thanks everyone for your help and feedback getting this ready to merge! :heart: