Transport-for-the-North / caf.space

https://cafspace.readthedocs.io/en/stable/
Other
2 stars 2 forks source link

Logging change #11

Closed isaac-tfn closed 1 year ago

isaac-tfn commented 1 year ago

Describe Changes

Task Checklist

codecov[bot] commented 1 year ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Files Coverage Δ
src/caf/space/weighted_funcs.py 100.00% <100.00%> (ø)
src/caf/space/ui.py 22.11% <0.00%> (+0.13%) :arrow_up:

:loudspeaker: Thoughts on this report? Let us know!.

isaac-tfn commented 1 year ago

I think tests are only failing here because LogHelper isn't available from toolkit yet. Requesting a review, probably from @wsp-mbuckley as this is an implementation of his LogHelper

isaac-tfn commented 1 year ago

@wsp-mbuckley I only just remembered the suggested changes in this review, could you take a quick 5 min look and (hopefully) approve?

wsp-mbuckley commented 1 year ago

@wsp-mbuckley I only just remembered the suggested changes in this review, could you take a quick 5 min look and (hopefully) approve?

There were 3 comments where I said use __package__ instead of "SPACE" for the getLogger functions which haven't been changed yet, please could you do those. I think everything else looks good

isaac-tfn commented 1 year ago

@wsp-mbuckley I only just remembered the suggested changes in this review, could you take a quick 5 min look and (hopefully) approve?

There were 3 comments where I said use __package__ instead of "SPACE" for the getLogger functions which haven't been changed yet, please could you do those. I think everything else looks good

I thought I'd changed them - I'll make sure I've pushed all my changes sorry

isaac-tfn commented 1 year ago

@wsp-mbuckley I might be missing something but I see 4 places you've requested "SPACE" changes to package and I've made those changes. All of the instances seem to be in 'ui' and 'main' so if there are more places it needs changing I don't think I can see those comments

wsp-mbuckley commented 1 year ago

@wsp-mbuckley I might be missing something but I see 4 places you've requested "SPACE" changes to package and I've made those changes. All of the instances seem to be in 'ui' and 'main' so if there are more places it needs changing I don't think I can see those comments

Sorry Isaac, you're right when I looked at the comments in VS code it linked back to the state of the file at the commit when I made the comment (I guess trying to be helpful?)

isaac-tfn commented 1 year ago

@wsp-mbuckley I might be missing something but I see 4 places you've requested "SPACE" changes to package and I've made those changes. All of the instances seem to be in 'ui' and 'main' so if there are more places it needs changing I don't think I can see those comments

Sorry Isaac, you're right when I looked at the comments in VS code it linked back to the state of the file at the commit when I made the comment (I guess trying to be helpful?)

No problem - I think suggestions is relatively new? (or you and Ben have both found the feature at about the same time) so maybe not quite optimised in VS code. Thanks for the review will merge now.