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.

Sign up for GitHub

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

Jump to bottom

Updated docstring in TimeSeries #6829

Merged
merged 1 commit into from
Mar 24, 2023
Merged

Updated docstring in TimeSeries #6829

merged 1 commit into from
Mar 24, 2023

Conversation

davnish
Copy link
Contributor

@davnish davnish commented Mar 9, 2023 •
edited

This updates the docstring for Timeseries.
This will enable us to use the help(TimeSeries) to see the supported source packages for TimeSeries.

Merging this PR:
Closes #4168

@davnish davnish requested a review from a team as a code owner March 9, 2023 15:24
@nabobalis
Copy link
Contributor

Thank you for the PR @davnish, unfortunately this does not work.

The names are actually to the source keyword and not the inputs line.

The issue with this approach is if we add more sources, we would have to manually change the text every time.

@nabobalis nabobalis marked this pull request as draft March 9, 2023 17:25
@nabobalis nabobalis added the timeseries Affects the timeseries submodule label Mar 9, 2023
@nabobalis nabobalis removed the request for review from a team March 18, 2023 18:12
@davnish
Copy link
Contributor Author

davnish commented Mar 18, 2023 •
edited

@nabobalis Sorry for the delay, I was looking for a better way to approach this issue. I have updated my approach. I have added an __all__ in __init__.py of sunpy.timeseries.sources which will keep track of the data source packages and when we will add a new source we can just update the __all__. And I have added dynamic docstring which will show the list __all__ in the input parameter. I did some testing too help(TimeSeries) work fine. is this okay or should I change my approach?

nabobalis
nabobalis reviewed Mar 18, 2023
View reviewed changes
sunpy/timeseries/sources/__init__.py Outdated Show resolved Hide resolved
sunpy/timeseries/timeseries_factory.py Outdated Show resolved Hide resolved
sunpy/timeseries/timeseries_factory.py Outdated Show resolved Hide resolved
sunpy/timeseries/timeseries_factory.py Outdated Show resolved Hide resolved
@nabobalis
Copy link
Contributor

@nabobalis Sorry for the delay, I have updated my approach. I have added an __all__ in __intit__.py of sunpy.timeseries.sources which will keep track of the data source packages and when we will add a new source we can just update the __all__. will this work?

It helps but the class names do not match the value used by the source keyword. We would need to extract them.

@davnish
Copy link
Contributor Author

davnish commented Mar 18, 2023

Okay I was confused, I thought that the class names were the input lines. How should I extract the value used by source keywords?

@nabobalis
Copy link
Contributor

I'm not sure where the source is stored in each class, you'll have to check and see if we can access it

@davnish
Copy link
Contributor Author

davnish commented Mar 19, 2023

Okay, I cross checked,
['eve', 'esp' , 'gbmsummary', 'xrs', 'lyra', 'noaaindices', 'noaapredictindices', 'norh', 'rhessi']
These are sources currently supported. where should I add this list? and I will change the inputs keyword to sources

@nabobalis
Copy link
Contributor

They go with the source keyword. Ideally they should be added to the type location following the numpydoc style.

@davnish
Copy link
Contributor Author

davnish commented Mar 19, 2023

The issue with this approach is if we add more sources, we would have to manually change the text every time.

Okay, I will add them to type location following the numpy docstyle. but wont this approach make the docstring static?

@nabobalis
Copy link
Contributor

The issue with this approach is if we add more sources, we would have to manually change the text every time.

Okay, I will add them to type location following the numpy docstyle. but wont this approach make the docstring static?

Yes but you'll need to work out a method to make that dynamic.

@davnish
Copy link
Contributor Author

davnish commented Mar 19, 2023

I have added a source list in init.py, which we can update when we add a new source. Are these changes right to do?

@davnish
Copy link
Contributor Author

davnish commented Mar 20, 2023

Are these changes okay? @nabobalis

@nabobalis nabobalis added No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) No Changelog Entry Needed Minor Change PR only needs one approval to merge labels Mar 20, 2023
@davnish
Copy link
Contributor Author

davnish commented Mar 21, 2023

@nabobalis I did the changes you asked for. I am not sure about the last change, I have done the changes but is this the change your asked for?

@davnish
Copy link
Contributor Author

davnish commented Mar 21, 2023

pre-commit.ci autofix

@davnish davnish force-pushed the updated_docstring branch 2 times, most recently from 46773b9 to 18fbb43 Compare March 21, 2023 14:13
@nabobalis
Copy link
Contributor

nabobalis commented Mar 21, 2023 •
edited

The precommit needs fixing but otherwise, I think its ready for reviews.

@nabobalis nabobalis marked this pull request as ready for review March 21, 2023 15:22
@davnish
Copy link
Contributor Author

davnish commented Mar 21, 2023

pre-commit.ci autofix

@davnish
Copy link
Contributor Author

davnish commented Mar 21, 2023

The precommit needs fixing but otherwise, I think its ready for reviews.

Thank you, for helping and guiding me. I did a lot of mistakes and didn't understand what you referred to earlier, but i learned a lot from this PR. Thank You once again @nabobalis

@davnish
Copy link
Contributor Author

davnish commented Mar 21, 2023

pre-commit.ci autofix

@nabobalis nabobalis added the Merge When CI Passes Hit that merge button when it's all green! label Mar 24, 2023
@nabobalis nabobalis merged commit 00557c3 into sunpy:main Mar 24, 2023
@nabobalis
Copy link
Contributor

Thanks for the PR @davnish

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabobalis nabobalis nabobalis approved these changes

Assignees
No one assigned
Labels
Merge When CI Passes Hit that merge button when it's all green! Minor Change PR only needs one approval to merge No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) No Changelog Entry Needed timeseries Affects the timeseries submodule
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide easier access to valid TimeSeries source keywords
2 participants
@davnish @nabobalis