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 modules #68

Merged
merged 3 commits into from
Sep 6, 2024
Merged

Refactor modules #68

merged 3 commits into from
Sep 6, 2024

Conversation

greglucas
Copy link
Collaborator

This is a major refactor, breaking the xtcedef.py module into separate entities.

I went with:

  • calibrators
  • encodings
  • exceptions
  • matches (comparisons? other ideas for this?)
  • packets (contains the sequence container and packet data definitions, would "containers" be a better module name?)
  • parameters (contains the Parameter (only one) and ParameterTypes)

I'm happy to move things around more/less too, just let me know your preferences. There is some fighting going on with circular imports depending on how we structure this, so we do need to make sure not to couple the modules too closely.

closes #34

Checklist

  • Changes are fully implemented without dangling issues or TODO items
  • Deprecated/superseded code is removed or marked with deprecation warning
  • Current dependencies have been properly specified and old dependencies removed
  • New code/functionality has accompanying tests and any old tests have been updated to match any new assumptions
  • The changelog.md has been updated

@greglucas greglucas requested a review from medley56 as a code owner September 4, 2024 22:50
@medley56
Copy link
Owner

medley56 commented Sep 5, 2024

I think I borked this PR by merging your Packet/PacketData unification PR. Sorry about that. This is going to be a pain if we're changing code inside the modules. After my MIL-1750A PR I'm done for a while. Can we merge that, rebase this, and then merge it?

@medley56
Copy link
Owner

medley56 commented Sep 5, 2024

@greglucas I'll approve this as soon as you can resolve the rebase.

@greglucas
Copy link
Collaborator Author

@medley56, this should be good to go with a rebase now and I can do large find/replace to move things around pretty easily if we want to change any namespaces.

My two questions for name changes (better suggestions welcome too!):

  • packets.py -> containers.py ?
  • matches.py -> comparisons.py ? (matches was slightly annoying because you used matches as a variable name and so we were shadowing the imported module name. There is also a new match syntax in Python 3.10+ that we might want to try to avoid confusion with)

@medley56
Copy link
Owner

medley56 commented Sep 6, 2024

@greglucas I like comparisons.py over matches.py. I think I like parsing.py or maybe parseables.py more than containers.py because that module contains the Parsable protocol and everything in there seems to be related to actually parsing data.

@medley56 medley56 merged commit 3744845 into medley56:main Sep 6, 2024
7 checks passed
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.

Refactor xtcedef.py into sub modules
2 participants