Skip to content

♻️ PR modifications#1

Merged
JuliaSprenger merged 9 commits into
JuliaSprenger:add/imp_expfrom
redcap-tools:pr/210
Mar 21, 2022
Merged

♻️ PR modifications#1
JuliaSprenger merged 9 commits into
JuliaSprenger:add/imp_expfrom
redcap-tools:pr/210

Conversation

@pwildenhain
Copy link
Copy Markdown

@pwildenhain pwildenhain commented Mar 17, 2022

Made a few changes here that I think will help the tests to pass, as well as some restructuring to fit the existing architecture.

Let's merge this into your PR and see how it turns out 🤞🏻

Some things left on the to do list before merging this in:

  • Add repeating instrument/event to the doctest project to have an actual doctest example
  • Add the repeating.py module to the docs site, update docs site.
  • Add unit tests for repeating instruments
  • probably other things I'm forgetting but it will be fine 😛

Again thanks so much @JuliaSprenger for adding this in, I know a lot has changed since the last time you worked on PyCap. I tried my best to make this project contributor friendly, while still having cool things like integration tests, but clearly we still have a little ways to go

Also if you wouldn't mind, can you share your experience with learning the new codebase? Was it easy enough to pick up? Any snags/things that tripped you up along the way? Want to make sure I iron out as many kinks as possible before releasing 2.0

Previously the doctests would run by default with the `doctest-plus` option enabled. By removing that option, it will need to be added in whenever someone wants to run the doctests locally. But this makes it actually possible for the CI to skip the doctests when running from a fork
@pwildenhain pwildenhain marked this pull request as draft March 17, 2022 15:57
@pwildenhain pwildenhain marked this pull request as ready for review March 17, 2022 16:07
Julia Sprenger and others added 2 commits March 17, 2022 14:08
Since they are under a separate section in the API playground, i'd prefer to keep them separated here, also because they can export events as well as instruments.
numpy recently dropped support and so now it's pretty much impossible to please all the version restrictions with related packages. With python 3.7 out of teh way, we no longer need the typing_extensions package, since everything we were using it already in typing by python 3.8
@pwildenhain
Copy link
Copy Markdown
Author

pwildenhain commented Mar 18, 2022

Ok I'm done messing around, it's good to merge at this point 😄

You don't really need to pay attention to anything after the merge commit, those commits were just to keep your deps/ci up to date with what's in master now.

After merging this, you'll want to re-run poetry install -E data_science to grab all the new deps

@JuliaSprenger
Copy link
Copy Markdown
Owner

Thanks for the refactoring and cleanup! I will merge now. Let's see if the tests will succeed this time.

@JuliaSprenger JuliaSprenger merged commit cdb91c2 into JuliaSprenger:add/imp_exp Mar 21, 2022
@pwildenhain
Copy link
Copy Markdown
Author

@JuliaSprenger I meant to ask -- can you share your experience with learning the new codebase? Was it easy enough to pick up? Any snags/things that tripped you up along the way? Want to make sure I iron out as many kinks as possible before releasing 2.0

@JuliaSprenger
Copy link
Copy Markdown
Owner

The general structure of the codebase is easy to grasp, just finding the way during debugging / in tracebacks is sometimes a bit tricky as these are very verbose and it's not always easy to distinguish important from non-relevant information there. For the tests it's similar since there's no 1-1 mapping between methods and tests it's a bit harder to find the right place.
typing as well as poetry was new to me, but I they seem very useful and not too complicated to get started with.

@pwildenhain
Copy link
Copy Markdown
Author

Great point on the tests, it'll be good to get your feedback on adding unit tests to see if there's any restructuring/documentation that should be added to make this less confusing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants