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

changed from key error to value error in from_sbdb #611

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Himanshu-Garg
Copy link
Contributor

commented Mar 18, 2019

as per issue #609
now when no. of objects are more than 1, instead of key error, its showing Value error as discussed in comments section of #609

here is the new output ...
Screenshot (156)

pls review!!!

Himanshu-Garg added some commits Mar 18, 2019

@codecov

This comment has been minimized.

Copy link

commented Mar 18, 2019

Codecov Report

Merging #611 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #611      +/-   ##
==========================================
+ Coverage   85.03%   85.08%   +0.05%     
==========================================
  Files          51       51              
  Lines        2386     2394       +8     
  Branches      183      184       +1     
==========================================
+ Hits         2029     2037       +8     
  Misses        308      308              
  Partials       49       49
Impacted Files Coverage Δ
src/poliastro/twobody/orbit.py 90.44% <100%> (+0.26%) ⬆️

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 b457cda...5850ed0. Read the comment docs.

Himanshu-Garg added some commits Mar 18, 2019

@Juanlu001
Copy link
Member

left a comment

Hi @Himanshu-Garg, sorry for the delay and thanks for this pull request! I added a couple of comments. Also, this will need a rebase on current master.

with pytest.raises(ValueError) as e:
Orbit.from_sbdb(name="Halley")

assert str(e.value)[0] == "2"

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 29, 2019

Member

Can we test the full message?

epoch=epoch.tdb,
plane=Planes.EARTH_ECLIPTIC,
)
except KeyError:

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 29, 2019

Member

I don't like this way of using an error to control the flow. Perhaps could do instead:

if "count" in obj:
    # Handle the error
    raise ValueError(...)

# No need of else block, just do the rest of the stuff
a = obj["orbit"][...]
...

This comment has been minimized.

Copy link
@Ivan9528

Ivan9528 Apr 9, 2019

I will take care of these changes asap! The idea is pretty much the same as in issue #609, right? Raise a proper message (not a key) when an error is produced.

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Apr 9, 2019

Member

Correct!

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

@Himanshu-Garg are you still working on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.