Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix Issues with Shifting of Insets to Position and String to Decimal Converter #218
base: main
Are you sure you want to change the base?
Fix Issues with Shifting of Insets to Position and String to Decimal Converter #218
Changes from 4 commits
245e901
5b989f0
a3e19b9
e493c85
76495a5
f0536c9
d291175
d8d6872
08761ee
63dabb7
073ae2b
5e2d047
47a2667
0c5c00c
b83a2b9
3f0cd84
41634e8
5ff5b2b
bb44ca0
89b589f
0a4a0ac
6dc2b26
3b1ad37
ac950c8
5a240b6
41ce02e
c7dd6f3
3fa6e02
03507bf
527f4c7
409fc33
3046fd4
f7f9b5a
07fc7e6
7dff687
ae2baf5
64aed88
ab67eff
2009185
6bec777
e9de287
c8920df
118c861
f44f74a
0166544
28b23e4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We should change this to follow the style of the rest of our
std::cerr
statements.That is:
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.
Okay, I have added an issue for this.
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.
Maybe it would be better to just store this in a variable? Or do you mean the current area of the
geo_divs_transformed
, in which case probably we would want to change the name somehow.Maybe "original_unit"?
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.
Storing the area in a variable would require us to update that variable each time we update the
geo_divs
, which would require extra book keeping in many places, and this will increase the possibilities of bugs.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.
Should we make this statement lighter. Or actually even remove it? Did you put it so that we can remember where this error came from?
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.
I feel we should not remove it; this way we know that the
--output_equal_area
flag did not work for the given map. It is true that the statement contains redundant information because the first statement technically implies the second statement given the context. However, for a new user, it might not be obvious; and we do plan to display error messages to the users on the website.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.
Maybe we should name the bools? @mgastner's opinion on style may be helpful here.
The alternative is we keep it like this and rely on IDE hints.
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.
Good idea. Let's discuss it more.
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.
On a side note, I wonder who is naming these exit codes.