You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/07/07 21:28:58 UTC
[GitHub] [avro] Diptonil opened a new pull request, #1756: Shifted integer and long constants to constants.py
Diptonil opened a new pull request, #1756:
URL: https://github.com/apache/avro/pull/1756
The changes made are:
- Code altered according to PEP 8 conventions. Files changed are: io.py, constants.py, schema.py, codecs.py, datafile.py and compatibility.py.
- General constants INT_MIN_VALUE, INT_MAX_VALUE, LONG_MIN_VALUE & LONG_MAX_VALUE shifted from schema.py to constants.py for clarity and better organization.
_(This is my first attempt to contribute to any open source project, not only this project. If I have made any errors or have misunderstood any step, please forgive me. I would be happy to adhere to any suggestions)_
### Jira
- [x] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO-3571) issues and references them in the PR title. For example, "[AVRO-3571]: Python code refactor and rearrangement"
- https://issues.apache.org/jira/browse/AVRO-3571
- No dependencies are being added.
### Tests
- [x] My PR does not need testing because I have only moved certain constants from schema.py to constants.py and have refactored code to better adhere to the PEP 8 style guide.
### Commits
- [x] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [x] My commit doesn't add any functionality whatsoever.
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] Diptonil commented on pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
Diptonil commented on PR #1756:
URL: https://github.com/apache/avro/pull/1756#issuecomment-1178575426
Thanks for the heads up. Will go through the code more extensively using black...
Also, my last commit seems to have passed all workflows. Do review it whenever time permits and merge it if its is alright...
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by "kojiromike (via GitHub)" <gi...@apache.org>.
kojiromike commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r1260493761
##########
lang/py/avro/schema.py:
##########
@@ -51,7 +51,15 @@
import avro.constants
Review Comment:
@Diptonil you may make as many commits on this PR as you think are necessary.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
kojiromike commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r916688997
##########
lang/py/avro/io.py:
##########
@@ -419,7 +413,7 @@ class BinaryEncoder:
def __init__(self, writer: IO[bytes]) -> None:
"""
- writer is a Python object on which we can call write.
+ Writer is a Python object on which we can call write.
Review Comment:
Since this refers to the "writer" parameter, it should remain lowercase. But you could consider backticks, as in…
```
"""
`writer` is an object on which we can call `write`.
"""
```
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] Diptonil commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
Diptonil commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r940120774
##########
lang/py/avro/schema.py:
##########
@@ -51,7 +51,15 @@
import avro.constants
Review Comment:
Yes, the import of `avro.constants` may be removed since certain constants are being imported separately.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] Diptonil commented on pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
Diptonil commented on PR #1756:
URL: https://github.com/apache/avro/pull/1756#issuecomment-1178580264
I shall get back after properly scrutinizing the work soon.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KalleOlaviNiemitalo commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r916506878
##########
lang/py/avro/io.py:
##########
@@ -940,9 +931,7 @@ def skip_record(self, writers_schema: avro.schema.RecordSchema, decoder: BinaryD
self.skip_data(field.type, decoder)
def _read_default_value(self, field_schema: avro.schema.Schema, default_value: object) -> object:
- """
- Basically a JSON Decoder?
- """
+ """Basically a JSON Decoder?"""
Review Comment:
While looking at this, I found what appears to be a bug in `_read_default_value`. Filed as [[AVRO-3572] Python encodes default value of bytes field as UTF-8](https://issues.apache.org/jira/browse/AVRO-3572).
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
kojiromike commented on PR #1756:
URL: https://github.com/apache/avro/pull/1756#issuecomment-1178366707
Don't worry about stylistic comments from flake8. The style for the Avro Python code is managed by [black](https://black.readthedocs.io/en/stable/).
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
kojiromike commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r925859992
##########
lang/py/avro/constants.py:
##########
@@ -57,3 +56,8 @@
)
VALID_TYPES = PRIMITIVE_TYPES + NAMED_TYPES + ("array", "map", "union", "request", "error_union")
+
+INT_MIN_VALUE = -(1 << 31)
+INT_MAX_VALUE = (1 << 31) - 1
+LONG_MIN_VALUE = -(1 << 63)
+LONG_MAX_VALUE = (1 << 63) - 1
Review Comment:
I am fairly ambivalent about this change, because we don't appear to use these particular constants in more than one module. They may as well be "private". However, if it's useful externally for some reason, then I'm not against it.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r940111289
##########
lang/py/avro/schema.py:
##########
@@ -51,7 +51,15 @@
import avro.constants
Review Comment:
Ping!
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on PR #1756:
URL: https://github.com/apache/avro/pull/1756#issuecomment-1178279473
Hello!
> (This is my first attempt to contribute to any open source project, not only this project. If I have made any errors or have misunderstood any step, please forgive me. I would be happy to adhere to any suggestions)
That's great, thanks for joining the community! You should be aware that we're a bit slow in reviewing PRs right now -- it's both summertime and working towards a 1.11.1 release. We'll get to this as soon as we can.
In the meantime, it looks like a lint failed with our build scripts... can you take a look at why this happens (and only with python 3.10?)
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r926356921
##########
lang/py/avro/schema.py:
##########
@@ -51,7 +51,15 @@
import avro.constants
Review Comment:
Should this be removed ?
CodeQL says that `avro.constants` is imported twice
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by "kojiromike (via GitHub)" <gi...@apache.org>.
kojiromike commented on PR #1756:
URL: https://github.com/apache/avro/pull/1756#issuecomment-1643041003
I resolved the issue with flake8 in [AVRO-3807](https://issues.apache.org/jira/browse/AVRO-3807). If you run `autoflake` on your branch it should help clean up the conflicts.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
kojiromike commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r916803181
##########
lang/py/avro/io.py:
##########
@@ -204,11 +204,12 @@ def _map_iterator(node: ValidationNode) -> ValidationNodeGeneratorType:
class BinaryDecoder:
"""Read leaf values."""
+
_reader: IO[bytes]
def __init__(self, reader: IO[bytes]) -> None:
"""
- Reader is a Python object on which we can call read, seek, and tell.
+ `reader` is a Python object on which we can call `read`, seek, and tell.
Review Comment:
For consistency, `seek` and `tell` should be backtick-quoted as well.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] Diptonil commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
Diptonil commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r940121261
##########
lang/py/avro/schema.py:
##########
@@ -51,7 +51,15 @@
import avro.constants
Review Comment:
Should another commit be made?
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KalleOlaviNiemitalo commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r916480081
##########
lang/py/avro/io.py:
##########
@@ -205,12 +204,11 @@ def _map_iterator(node: ValidationNode) -> ValidationNodeGeneratorType:
class BinaryDecoder:
"""Read leaf values."""
-
_reader: IO[bytes]
def __init__(self, reader: IO[bytes]) -> None:
"""
- reader is a Python object on which we can call read, seek, and tell.
+ Reader is a Python object on which we can call read, seek, and tell.
Review Comment:
If that refers to the `reader` parameter, then perhaps it would be better to keep it in lower case. Not a strong opinion
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] Diptonil commented on pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
Diptonil commented on PR #1756:
URL: https://github.com/apache/avro/pull/1756#issuecomment-1178350050
Hello!
I just enabled the Flake8 linter for my project. There are a lot of "line too long" warnings for every file. I'm assuming that I should leave that be? There are some other warnings as well - unused variables and imports. Would it be pertinent of me to clear off those warnings?
Thank you. I understand if the review process is slow. That's totally fine.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on a diff in pull request #1756: AVRO-3571: Shifted integer and long constants to constants.py
Posted by GitBox <gi...@apache.org>.
kojiromike commented on code in PR #1756:
URL: https://github.com/apache/avro/pull/1756#discussion_r916689922
##########
lang/py/avro/io.py:
##########
@@ -205,12 +204,11 @@ def _map_iterator(node: ValidationNode) -> ValidationNodeGeneratorType:
class BinaryDecoder:
"""Read leaf values."""
-
_reader: IO[bytes]
def __init__(self, reader: IO[bytes]) -> None:
"""
- reader is a Python object on which we can call read, seek, and tell.
+ Reader is a Python object on which we can call read, seek, and tell.
Review Comment:
I agree. See my comment below about `writer`.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org