search

SHADE-AI / daidepp

Repo to store universal communication specification
2 stars 7 forks source link

Use `tuple` instead of `frozenset` (and other miscellaneous changes) #92

Closed aphedges closed 1 year ago

aphedges commented 1 year ago

I made a large number of small, unrelated changes, and I figured it didn't make sense to make PRs for each one.

The biggest change is to use tuple instead of frozenset/set in DAIDE objects. I made my argument in the commit message for that commit. Please let me know if you have any objections.

If/when this is merged, please do not to squash these commits. They are much too unrelated to be combined.

aphedges commented 1 year ago

This PR hasn't been reviewed yet, so I figured I should push some new changes I made today in response to problems I discovered.

aphedges commented 1 year ago

This PR hasn't been reviewed yet, so I've pushed some additional changes. I cleaned up the commit history while I was at it because I figured no one had checked out this branch yet.

I would very much appreciate if this could be reviewed in the near future so I can upgrade to it.

mjspeck commented 1 year ago

I'm gonna look at this tomorrow. I think most of these changes are fine, but I may propose a small change to get the best of frozenset and tuple.

aphedges commented 1 year ago

@mjspeck, thanks! I figured we'd need some discussion over the frozenset vs tuple change.