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

Work after switching to attrs #45

Open
4 of 9 tasks
astrojuanlu opened this issue Nov 6, 2019 · 4 comments
Open
4 of 9 tasks

Work after switching to attrs #45

astrojuanlu opened this issue Nov 6, 2019 · 4 comments

Comments

@astrojuanlu
Copy link
Member

astrojuanlu commented Nov 6, 2019

(Copied from #44)

This already passes all the tests locally.

Things that will have to be reworked:

  • Shortcuts, converters

Things I removed and I'd like to bring back:

  • Docstrings

Things I'd like to review:

  • Defaults (we should probably be using default=None everywhere)
  • isinstance (we are inconsistently using positive and negative checks)
  • Validation (we use __attrs_post_init__ but we could use @x.validator instead http://www.attrs.org/en/stable/init.html#decorator)
  • Mixins (are they necessary?)
  • KNOWN_PROPERTIES (can I iterate over attrs properties now instead?)
  • Enumerations (including missing StripeOrientationValue)

Things I'd like to add in the future:

@noc0lour
Copy link

noc0lour commented Dec 4, 2019

Note: The example in README is not working since CZMLWidget now expectes a Document() to be passed instead of a list. Now either a Document(simple) has to be passed or CZMLWidget has to do it in the initalizer.

Edit: For simple it works because it's a Document now, but it doesn't work for a list of packets. I don't know what would be the right way here. Maybe just a documentation issue.

@astrojuanlu
Copy link
Member Author

@noc0lour I'd say that CZMLWidget should always expect a Document, do you see any inconsistency in the README or any docstring?

@noc0lour
Copy link

noc0lour commented Dec 4, 2019

No, somehow I expected in poliastro to be able to stick results from the CZML_extractor directly into a CZMLWidget, which is a poliastro issue not a czml3 issue.

@astrojuanlu
Copy link
Member Author

Oh, I see! Yep, CZMLExtractor should produce a Document with a proper Preamble.

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

No branches or pull requests

2 participants