You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/08/04 07:07:42 UTC
[GitHub] [iceberg] nastra opened a new pull request, #5434: Core: Highlight property names with single quotes in JsonUtil error messages
nastra opened a new pull request, #5434:
URL: https://github.com/apache/iceberg/pull/5434
Sometimes one has to read the error message twice to figure out which
property name was affected, thus highlighting the property name in the
error message should make this clearer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #5434: Core: Highlight property names with single quotes in JsonUtil error messages
Posted by GitBox <gi...@apache.org>.
nastra commented on PR #5434:
URL: https://github.com/apache/iceberg/pull/5434#issuecomment-1207735685
> I think it's better to put names like this after a `:` instead of adding characters that can get copied and moved to other places. Putting names after `:` still allows you to see when they are empty or null (e.g. `Cannot find field: `) without adding non-whitespace characters.
That makes sense, but how would we want to handle something like `Cannot parse %s from non-array value: %s`?
I feel that `Cannot parse '%s' from non-array value: %s` improves readability. Or we could add the property name to the end in this particular case to have `Cannot parse from non-array value: %s: %s`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5434: Core: Highlight property names with single quotes in JsonUtil error messages
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5434:
URL: https://github.com/apache/iceberg/pull/5434#issuecomment-1214419944
> we could add the property name to the end in this particular case to have `Cannot parse from non-array value: %s: %s`.
I prefer this approach to quotes. When needed, reword to make it clear that something is missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #5434: Core: Highlight property names with single quotes in JsonUtil error messages
Posted by GitBox <gi...@apache.org>.
nastra commented on PR #5434:
URL: https://github.com/apache/iceberg/pull/5434#issuecomment-1214738323
@rdblue updated and pushed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #5434: Core: Highlight property names with single quotes in JsonUtil error messages
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5434:
URL: https://github.com/apache/iceberg/pull/5434#issuecomment-1207497436
> I think it's better to put names like this after a `:` instead of adding characters that can get copied and moved to other places. Putting names after `:` still allows you to see when they are empty or null (e.g. `Cannot find field: `) without adding non-whitespace characters.
I know I approved this PR, but I do tend to agree with this argument.
In particular, I find value in having any variable / changing value in a log (in this case the field name) after `:` so that there is a common leading string to search for in the logs.
Combined with the first line of the stack trace, it should be clear in context, for example, that an integer was attempting to be parsed as we're in the function `getInt` etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5434: Core: Highlight property names with single quotes in JsonUtil error messages
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5434:
URL: https://github.com/apache/iceberg/pull/5434#issuecomment-1207496575
I think it's better to put names like this after a `:` instead of adding characters that can get copied and moved to other places. Putting names after `:` still allows you to see when they are empty or null (e.g. `Cannot find field: `) without adding non-whitespace characters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #5434: Core: Put property names at the end in JsonUtil error messages
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5434:
URL: https://github.com/apache/iceberg/pull/5434
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org