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 times.py #359

Open
cleder opened this issue Oct 5, 2024 · 11 comments · May be fixed by #368
Open

Refactor times.py #359

cleder opened this issue Oct 5, 2024 · 11 comments · May be fixed by #368

Comments

@cleder
Copy link
Owner

cleder commented Oct 5, 2024

Refactor times to use the registry.

  • Add new classes When, Begin and End
  • Remove the etree_element and _get_kwargs methods from TimeStamp and TimeSpan
  • Add new function following the GetKWArgs Protocol to parse the KmlDateTime from XML
  • Add new function following the SetElement Protocol to create the etree representation
  • Register the functions for the new classes
  • TimeStamp to optionally take the above when
  • TimeSpan to optionally take begin and end
@rishitc
Copy link

rishitc commented Oct 5, 2024

Hi @cleder,
I'm interested in working on this issue. Can you please assign it to me?

Thanks

@cleder
Copy link
Owner Author

cleder commented Oct 5, 2024

Thanks @rishitc

@cleder cleder added this to the Version 1.0 milestone Oct 5, 2024
@rishitc
Copy link

rishitc commented Oct 11, 2024

Hi @cleder,
I want to update you that I'm working on this.
Progress is a bit slow, but I hope to have an initial patch soon.

Thanks

@cleder
Copy link
Owner Author

cleder commented Oct 15, 2024

Can you document how to do it? For me, it is obvious, so I need an outsider's view on it to improve the documentation 😉

@cleder
Copy link
Owner Author

cleder commented Oct 15, 2024

BTW, make a draft pull request, ask questions, I am happy to provide some guidance

@rishitc
Copy link

rishitc commented Oct 16, 2024

Hi @cleder,

Can you document how to do it? For me, it is obvious, so I need an outsider's view on it to improve the documentation 😉

Sure, I can work more on that as part of the #343

BTW, make a draft pull request, ask questions, I am happy to provide some guidance

Thank you so much 😄
I'll put up my draft PR by Saturday 👍

@rishitc
Copy link

rishitc commented Oct 19, 2024

Hi @cleder,
In the file fastkml/times.py, TimeSpan already optionally accepts a begin and end of type Optional[KmlDateTime], so I think that sub-task is already taken care of, correct?

@rishitc
Copy link

rishitc commented Oct 19, 2024

@cleder, also the new classes When, Begin, and End must derive from _TimePrimitive so that they can access the super()._get_kwargs and super().etree_element methods, correct?

@cleder
Copy link
Owner Author

cleder commented Oct 19, 2024

I am not sure what you are trying to say, can you make a draft PR?

@rishitc
Copy link

rishitc commented Oct 19, 2024

Sounds good, I'll put up my draft PR in the next hour 👍

@rishitc rishitc linked a pull request Oct 19, 2024 that will close this issue
@rishitc
Copy link

rishitc commented Oct 19, 2024

Hi @cleder,
My draft PR is up 🎉


Also, here's the current checklist status of my PR:

  • Add new classes When, Begin and End
  • Remove the etree_element and _get_kwargs methods from TimeStamp and TimeSpan
  • Add new function following the GetKWArgs Protocol to parse the KmlDateTime from XML
    • The new classes will need to import from _TimePrimitive to support this, right?
  • Add new function following the SetElement Protocol to create the etree representation
    • The new classes will need to import from _TimePrimitive to support this, right?
  • Register the functions for the new classes
  • TimeStamp to optionally take the above when
  • TimeSpan to optionally take begin and end
    • I think this has already been completed in an older commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

2 participants