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

Capture missing solve-field error #116

Merged
merged 3 commits into from
Nov 17, 2017
Merged

Capture missing solve-field error #116

merged 3 commits into from
Nov 17, 2017

Conversation

wtgee
Copy link
Member

@wtgee wtgee commented Nov 13, 2017

  • Exit with code 127 if solve-field not found
  • Catch error from python and show appropriate message

Related to #113

@wtgee
* Exit with code 127 if solve-field not found
d93625a 
* Catch error from python and show appropriate message
@wtgee wtgee mentioned this pull request Nov 13, 2017
Merged
@wtgee wtgee requested a review from jamessynge November 13, 2017 09:52
@codecov
Copy link

codecov bot commented Nov 13, 2017 •
edited
Loading

Codecov Report

Merging #116 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #116   +/-   ##
========================================
  Coverage    84.39%   84.39%           
========================================
  Files           35       35           
  Lines         2345     2345           
  Branches       281      281           
========================================
  Hits          1979     1979           
  Misses         285      285           
  Partials        81       81
Impacted Files Coverage Δ
pocs/utils/images.py 92.45% <100%> (ø) ⬆️

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 172be05...c886b32. Read the comment docs.

wtgee
wtgee commented Nov 13, 2017
View reviewed changes
scripts/solve_field.sh Outdated
@@ -6,7 +6,7 @@ then
echo 2>&1 ""
echo 2>&1 "Please install astrometry into ${PANDIR}/astrometry to enable $0"
echo 2>&1 ""
exit 1
exit 127 # Command not found
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't actually know if this is the appropriate use of the error code 127 so don't mind switching back to 1 as long as we still catch above.

Copy link
Contributor

Choose a reason for hiding this comment

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

You might be better to go with 3. Codes 1, 2, 126 and 127 have meanings assigned by bash, so avoiding those codes may avoid confusion down the road. See: http://tldp.org/LDP/abs/html/exitcodes.html

jamessynge
jamessynge previously approved these changes Nov 15, 2017
View reviewed changes
Copy link
Contributor

@jamessynge jamessynge left a comment

Choose a reason for hiding this comment

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

LGTM

@jamessynge jamessynge dismissed their stale review November 15, 2017 14:29

Realized code should be changed.

@wtgee
Copy link
Member Author

wtgee commented Nov 15, 2017

I'll get this changed back. I also am not sure why I mixed the requirements.txt update into this but will probably remove.

@wtgee
* Changing to more appropriate error code for missing solve-field
c886b32 
* Reversing the change to matplotlib which should be in a separate issue
@wtgee wtgee merged commit 2d4e906 into panoptes:develop Nov 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamessynge jamessynge jamessynge left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wtgee @jamessynge