czml3 icon indicating copy to clipboard operation
czml3 copied to clipboard

Work after switching to attrs

Open astrojuanlu opened this issue 5 years ago • 4 comments

(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)~
  • [x] Validation (we use __attrs_post_init__ but we could use @x.validator instead http://www.attrs.org/en/stable/init.html#decorator)
  • [x] Mixins (are they necessary?)
  • [x] KNOWN_PROPERTIES (can I iterate over attrs properties now instead?)
  • [x] Enumerations (including missing StripeOrientationValue)

Things I'd like to add in the future:

  • [ ] Type annotations http://www.attrs.org/en/stable/types.html

astrojuanlu avatar Nov 06 '19 11:11 astrojuanlu

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.

noc0lour avatar Dec 04 '19 13:12 noc0lour

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

astrojuanlu avatar Dec 04 '19 13:12 astrojuanlu

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.

noc0lour avatar Dec 04 '19 14:12 noc0lour

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

astrojuanlu avatar Dec 04 '19 14:12 astrojuanlu