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

add get all platforms function and generator getting one platform to processing #84

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

19as
Copy link

@19as 19as commented Oct 15, 2021

  • Closes #xxxx
  • Tests added
  • Tests passed
  • Passes flake8 pyorbital
  • Fully documented

@19as
Copy link
Author

19as commented Oct 30, 2021

please, merge my request in trunk

@@ -39,3 +39,4 @@ nosetests.xml

# rope
.ropeproject
/.vs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/.vs
.vs

I don't think the / is needed.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Sorry about the delay, we've been kind of busy and sometimes pull requests get lost in the inbox. How do you plan on using this functionality?

I know very little about TLE files so correct me if I'm missing something, but it looks like your two functions are very similar and possibly don't do what they say. The top one says that it gets multiple platforms and the bottom one says it gets a single one. However, the second one is a generator so it actually yields all the platforms too.

Lastly, these functions are almost exactly the same code. Could they be combined? Or one could use the other? If the only difference between these is a list versus a generator then I would suggest having only the generator and leave it up to the user to call list(get_platform(tle_file)) instead of doing it for them.

pyorbital/tlefile.py Show resolved Hide resolved
pyorbital/tlefile.py Show resolved Hide resolved
pyorbital/tlefile.py Show resolved Hide resolved
pyorbital/tlefile.py Show resolved Hide resolved
@djhoese
Copy link
Member

djhoese commented Oct 31, 2021

Oh also could you try adding some tests for these functions?

@djhoese
Copy link
Member

djhoese commented Dec 14, 2021

@19as Any updates on this? I had some questions and comments that I was hoping you could answer before we move forward with this pull request. If you have questions for me let me know. If you don't reply in the next week or two then I will plan on closing this.

@19as
Copy link
Author

19as commented Jun 16, 2022

Sorry about the delay, we've been kind of busy and sometimes pull requests get lost in the inbox. How do you plan on using this functionality?

I know very little about TLE files so correct me if I'm missing something, but it looks like your two functions are very similar and possibly don't do what they say. The top one says that it gets multiple platforms and the bottom one says it gets a single one. However, the second one is a generator so it actually yields all the platforms too.

Lastly, these functions are almost exactly the same code. Could they be combined? Or one could use the other? If the only difference between these is a list versus a generator then I would suggest having only the generator and leave it up to the user to call list(get_platform(tle_file)) instead of doing it for them.

You're right. It is better to use one generator. I had to go through the file and, upon receiving one aircraft, process it

@19as
Copy link
Author

19as commented Jun 16, 2022

Oh also could you try adding some tests for these functions?

I will try to do it in the future

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

Successfully merging this pull request may close these issues.

None yet

3 participants