search

childmindresearch / wristpy

https://childmindresearch.github.io/wristpy/
GNU Lesser General Public License v2.1
4 stars 5 forks source link

Code review for Reinder to have fun #7

Closed Asanto32 closed 3 months ago

Asanto32 commented 6 months ago

@nx10 We have 3 green checkmarks. Feel free to review :)

codecov[bot] commented 6 months ago

Welcome to Codecov :tada:

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

ReinderVosDeWael commented 6 months ago

When everything is addressed we can merge and I make a few issues about the architecture so we can switch to smaller PRs that are easier to review (Unless @ReinderVosDeWael is opposed).

I'd recommend freezing development on this branch (after including Florian's minor suggestions) and then making branches that split off and merge into this branch. We should treat the main branch as production code i.e. it doesn't go to main until it receives full approval, but the feature branches merging into Adam_woking_v2 are evaluated based on only their completeness within their own scope.

@vitoetc, @gkiar are there any real harms caused by delaying merging this to main? Code coverage of 35% is sufficient by itself for me to recommend not releasing this yet, but resolving that puts us in a bind where either we 1) write tests for the current architecture that have to be rewritten upon making the required architectural changes (i.e. doubling work), or 2) we have to fix the architectural issues before getting to testing (i.e. delaying release).

gkiar commented 6 months ago

@ReinderVosDeWael — I support your recommendation here. Please sketch out the concrete roadmap for development here with @Asanto32 and @frey-perez , then run it by @vitoetc and myself for a final green-light.