get_obs_products method supports product_type parameter as string or list

[davidglt]

The get_obs_products method now checks the input argument product_type with the method _check_list_strings and allows using strings or lists

[pep8speaks]

Hello @davidglt! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2024-07-16 17:46:57 UTC

[jespinosaar]

cc @esdc-esac-esa-int

[davidglt]

Hi there! The last commit 61b8b14 only modifies from the file in the main branch the lines to include this PR information, but maybe I have something more wrong :) Cheers, David

[jespinosaar]

Hi @davidglt , please rebase your branch. It seems there is a conflict in CHANGES.rst file.

[davidglt]

Hi there! After rebase, we have the same problem with the CHANGES.rst file

Cheers, David

[bsipocz]

I'm not exactly sure what went wrong with the rebase here, I suspect it might not been rebased on the correct branch. Given that there are new conflicts I do a rebase again and will report back the command history I will have used.

[bsipocz]

Given that there were a lot of incorrect edits for the changelog, I removed those commits during the rebase and instead added a single clean on once the rest has been rebased. Here are the commands:

Make sure you have the astropy/astroquery repo added as a git remote (I call this astropy in my setup, but it maybe upstream in yours):

$ ~/munka/devel/astroquery [ESA_jwst-extend_get_obs_products|✚ 1…1⚑ 14] $ git remote -v
astropy git@github.com:astropy/astroquery.git (fetch)
astropy git@github.com:astropy/astroquery.git (push)
bsipocz git@github.com:bsipocz/astroquery.git (fetch)
bsipocz git@github.com:bsipocz/astroquery.git (push)
esdc-esac-esa-int   git@github.com:esdc-esac-esa-int/astroquery (fetch)
esdc-esac-esa-int   git@github.com:esdc-esac-esa-int/astroquery (push)

Have a fresh version of the main repo:

git fetch --all --prune

Interactive rebase on astropy/main. This will automatically get rid of the ~80 commits that you pulled in during the previous rebase. Also, in the interactive mode an editor will pop up. I removed all commits related to the changelog. The rebase was clean, I didn't need to resolve any conflicts, but if there are any, you read the instructions and do --continue until all commits are successfully rebased.

git rebase -i astropy/main

Once the rebase is done, I edited the changelog file and added that commit.

Now, the important part (which might have been the step that didn't worked previously) is to force push the branch back to github. I call the ESAC fork push esdc-esac-esa-int, you may have it as origin.

git push esdc-esac-esa-int HEAD:ESA_jwst-extend_get_obs_products -f

[bsipocz]

Also, @davidglt - if you add the email address you made the commits with to your github profile, these commits will properly show up as your contributions.

[davidglt]

Hi Brigitta, I am working to implement a new test for the product_type parameter as string or list

[davidglt]

Hi Brigitta, I've included a remote-test in docs/esa/jwst/jwst.rst with product_type as a list. Do you need anything else?

Thanks! David

[jespinosaar]

Thanks @davidglt! we still need to include the entry in the changelog.

[bsipocz]

Do you need anything else?

I'm travelling in the coming week so PR reviews and merges may be delayed. Sorry about that. With a quick look at the changes, I see that a test is still missing (# doctest: +SKIP skips running the code example). And the changelog changes will need to be squashed, but I'm more than happy to do that once I'm back in Seattle.

[davidglt]

Hi Brigitta, Now we have a green light! maybe the interpreter was confused :) I learned a lot from your suggestions! Cheers, David

[davidglt]

Hi Brigitta, Do you need anything more? Cheers, David

[jespinosaar]

Please add a test for product_type being a list.

Hi @davidglt , I will help you with the test.

[davidglt]

Hello! We have just implemented your two recommendations: 1) Replace "type(product_type is list" with isinstance(product_type, list) 2) Improve comments on the get_obs_products method for the output parameter

Cheers, David

[davidglt]

Please add a test for product_type being a list.

We have done an online test in docs/esa/jwst/jwst.rst, do you want an offline test too or is that enough? Cheers, David

[davidglt]

Please add a test for product_type being a list.

We have done an online test in docs/esa/jwst/jwst.rst, do you want an offline test too or is that enough? Cheers, David

Hi Brigitta, test offline for product_type as a list in test_get_obs_products included. Cheers, David

[jespinosaar]

Hi @bsipocz , I have seen the test is now included. I have checked it and seems to be working. The issue with the CI seems to be unrelated. Thanks @davidglt

[jespinosaar]

Hi @bsipocz , Many thanks for your feedback! We will be cleaning the code for all ESA modules in a different pull request, so maybe we can merge this PR and then create another one with changes for all of them. What do you think?

[bsipocz]

@jespinosaar - Yes, that sounds good to me. I'll rebase to fix the conflict, and then merge this one as is.

[bsipocz]

Thank you!