-
Notifications
You must be signed in to change notification settings - Fork 26
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
estimate-area command #104
Conversation
The build failures for most commits were before a merge with master which upgraded the Click version and fixed AssertionError quote issues from the existing code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @allisonzheng, looks great! just had a couple questions to help myself understand what's going on here and a couple small suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allisonzheng looks good! I added a few suggestions for changes. My biggest comment is that we should focus on unit testing the methods you've written instead of CLI tests -- they are harder to write and more prone to weirdness with things like string escaping. Some CLI tests should exist for usage, but the coverage of logical edge cases should be in module tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comments/questions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just left one small documentation suggestion.
🙇 @allisonzheng !
The estimate-area command inputs features and a precision and outputs the total area in square kilometers.