Skip to content
This repository has been archived by the owner on Dec 16, 2024. It is now read-only.

feat: number and epoch types, IntervalValue list parsing #151

Merged
merged 5 commits into from
Jul 17, 2024
Merged

feat: number and epoch types, IntervalValue list parsing #151

merged 5 commits into from
Jul 17, 2024

Conversation

vishwa2710
Copy link
Contributor

No description provided.

@vishwa2710 vishwa2710 mentioned this pull request Jul 16, 2024
Closed
Stoops-ML
Stoops-ML suggested changes Jul 16, 2024
View reviewed changes
Copy link
Collaborator

@Stoops-ML Stoops-ML left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

src/czml3/types.py Outdated Show resolved Hide resolved
vishwa2710
vishwa2710 commented Jul 16, 2024
View reviewed changes
src/czml3/types.py Outdated Show resolved Hide resolved
@vishwa2710
Copy link
Contributor Author

@Stoops-ML done!

Stoops-ML
Stoops-ML reviewed Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@Stoops-ML Stoops-ML left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small fix required.

src/czml3/types.py Outdated Show resolved Hide resolved
vishwa2710
vishwa2710 commented Jul 17, 2024
View reviewed changes
src/czml3/types.py Outdated Show resolved Hide resolved
@vishwa2710
Copy link
Contributor Author

🤦 thanks! This should be good.

@Stoops-ML
Copy link
Collaborator

Stoops-ML commented Jul 17, 2024 •
edited
Loading

The test test_interval_value() failed because the class CustomValue is not a subclass of BaseCZMLObject.

@Stoops-ML Stoops-ML self-requested a review July 17, 2024 08:30
@vishwa2710
Copy link
Contributor Author

Ah sorry for the mess. I wasn't running the tests locally as the setup is not containerized. I went through the motions and I've tested this locally. It should pass now 💯

@vishwa2710
Copy link
Contributor Author

Would we be opposed to containerizing this repo and adding a makefile with some simple commands for testing, formatting, etc.?

@Stoops-ML
Copy link
Collaborator

Need to run ruff format and ruff check --fix to format the code.

@Stoops-ML
Copy link
Collaborator

Would we be opposed to containerizing this repo and adding a makefile with some simple commands for testing, formatting, etc.?

Open an issue on this. I'd like to hear what others think about this.

@vishwa2710
Copy link
Contributor Author

Checked with ruff 👍

@Stoops-ML Stoops-ML merged commit 8c94a9c into poliastro:main Jul 17, 2024
7 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Stoops-ML Stoops-ML Stoops-ML approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vishwa2710 @Stoops-ML @vishwaLoft