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

Add CLI support for geometry input as string #381

Merged
merged 27 commits into from
Jan 31, 2021
Merged

Add CLI support for geometry input as string #381

merged 27 commits into from
Jan 31, 2021

Conversation

thomasyoung-audet
Copy link
Contributor

Adds CLI support for geometry input as string

@j08lue j08lue changed the title Iss350 Add CLI support for geometry input as string Jul 7, 2020
@codecov
Copy link

codecov bot commented Jul 9, 2020 •
edited
Loading

Codecov Report

Merging #381 (2d3fb0e) into master (ab56c5f) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   93.51%   93.66%   +0.15%     
==========================================
  Files           5        5              
  Lines         802      821      +19     
  Branches      169      172       +3     
==========================================
+ Hits          750      769      +19     
  Misses         34       34              
  Partials       18       18
Impacted Files Coverage Δ
sentinelsat/scripts/cli.py 100.00% <100.00%> (ø)
sentinelsat/sentinel.py 93.20% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab56c5f...2d3fb0e. Read the comment docs.

@thomasyoung-audet thomasyoung-audet mentioned this pull request Jul 10, 2020
Closed
kr-stn
kr-stn requested changes Jul 10, 2020
View reviewed changes
Copy link
Member

@kr-stn kr-stn left a comment

Choose a reason for hiding this comment

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

A nice feature added in this PR. I have some minor comments that need to be adressed.

Please make sure to also add this to the Changelog before we can get this merged.

sentinelsat/scripts/cli.py Outdated Show resolved Hide resolved
sentinelsat/sentinel.py Outdated Show resolved Hide resolved
valgur
valgur requested changes Jul 13, 2020
View reviewed changes
sentinelsat/scripts/cli.py Outdated Show resolved Hide resolved
@kr-stn
Copy link
Member

kr-stn commented Jul 13, 2020

Looks to me like the only thing missing is to adress the failing tests.

@valgur do you feel fine taking care of the final pass and the merging? I don't want to be the blocker here.

@thomasyoung-audet
Copy link
Contributor Author

Any problems with this or can it merge?

@kr-stn
Copy link
Member

kr-stn commented Jul 19, 2020

A changelog entry is missing, but everything else looks good from my side 👍

Anything you want to comment on @valgur?

@kr-stn
Copy link
Member

kr-stn commented Jul 24, 2020

If no-one has any objections I am going to merge this on Saturday and make it part of a minor release.

valgur
valgur requested changes Jul 25, 2020
View reviewed changes
sentinelsat/scripts/cli.py Outdated Show resolved Hide resolved
sentinelsat/scripts/cli.py Outdated Show resolved Hide resolved
sentinelsat/scripts/cli.py Outdated Show resolved Hide resolved
sentinelsat/sentinel.py Show resolved Hide resolved
@thomasyoung-audet
Add test to cover bad GeoJSON string input
a3f58b4
@thomasyoung-audet
Change help string for --geometry. Change JSON test to produce more a…
001c57b 
…ppropriate error. Change SyntaxError to JSONDecodeError. Optimize is_wkt function.
j08lue
j08lue reviewed Jul 31, 2020
View reviewed changes
sentinelsat/scripts/cli.py Outdated Show resolved Hide resolved
@thomasyoung-audet
Copy link
Contributor Author

Anyone want to take a look at this?

@j08lue
Copy link
Contributor

j08lue commented Aug 18, 2020

Anyone want to take a look at this?

You mean help fixing the failing test?

@thomasyoung-audet
Copy link
Contributor Author

I'll be honest the whole time I thought this was a problem with Travis and not something I could fix just playing with the code a bit... Felling a little dumb

@kr-stn
Copy link
Member

kr-stn commented Aug 18, 2020

Don't worry about it @thomasyoung-audet - we've all been there 😄

@valgur can you please re-review the requested changes, then we can get this merged.

@kr-stn
Copy link
Member

kr-stn commented Oct 11, 2020

Ping @valgur: Have your requested changes been adressed?

I regard the open change request as the final blocker before we can push this over the finish line.

@thomasyoung-audet
Copy link
Contributor Author

Hello @valgur, could we get this merged?? I fixed everything you suggested like 6 months ago

j08lue
j08lue approved these changes Jan 31, 2021
View reviewed changes
Copy link
Contributor

@j08lue j08lue left a comment

Choose a reason for hiding this comment

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

I can confirm that all our change requests have been followed up on and this is ready to merge. Thanks for seeing this through, @thomasyoung-audet.

@j08lue j08lue merged commit 9c6269c into sentinelsat:master Jan 31, 2021
@thomasyoung-audet thomasyoung-audet deleted the iss350 branch January 31, 2021 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valgur valgur valgur requested changes

@j08lue j08lue j08lue approved these changes

@kr-stn kr-stn kr-stn approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thomasyoung-audet @kr-stn @j08lue @valgur