Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor #599

Open
wants to merge 42 commits into
base: main
Choose a base branch
from
Open

refactor #599

wants to merge 42 commits into from

Conversation

cjdoris
Copy link
Collaborator

@cjdoris cjdoris commented Mar 29, 2025

Refactoring the codebase, which has got a bit out of hand.

  • move all internal code into new Internals submodule
  • all exported/public symbols defined in src/API/API.jl
  • ensure tests pass
  • use a common Internals.Common module which exports common functionality instead of needing to import functions from all over the place have each internal submodule export its internal API so other submodules can just do using ..Foo to get everything it needs
  • make internal submodules more granular - e.g. have submodules for PyList etc within Wrap
  • make internal function names shorter/clearer
  • maybe move testitems from test to src?

@hhaensel
Copy link
Contributor

hhaensel commented Mar 31, 2025

@cjdoris @MilesCranmer
Just saw a big refactoring PR. Does that mean that we would need to refactor this PR?

@cjdoris
Copy link
Collaborator Author

cjdoris commented Apr 11, 2025

@cjdoris @MilesCranmer Just saw a big refactoring PR. Does that mean that we would need to refactor this PR?

You will yeah but don't bother yet - this PR is likely to change some more.

Sorry for the delay on reviewing your PR, my time is currently limited and focused on bugs, improving code quality and working towards v1, rather than new features, but I will get to it.

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