Update csv_generator.py#2231
Conversation
added some features
ebeahan
left a comment
ebeahan
left a comment
There was a problem hiding this comment.
Type hints have been added to the code to specify the types of the function parameters and return values. This helps make the code easier to understand and maintain by providing clear information about the expected types of values
I actually see some of the type hinting removed in the changes. Is that intended, and, if so, what's the reasoning? As you stated, the type hints are intended to help.
The file opening mode has been updated to use 'w' instead of 'wb' when running Python 3 or above. This ensures compatibility with both Python 2 and Python 3.
Python 2.x support ended from the Python community in 2020. Since the ECS repo supports Python 3.8+, we could simplify by removing the check for Python 2.x vs. 3.x.
Removed unnecessary type casting: The str.lower() method is unnecessary when calling str() on a boolean value. It has been removed for simplicity.
This is the behavior I see in the REPL. I also don't actually see the .lower() method removed.
>>> str(True)
'True'
>>> str(True).lower()
'true'| @@ -1,33 +1,10 @@ | |||
| # Licensed to Elasticsearch B.V. under one or more contributor | |||
There was a problem hiding this comment.
I suspect this header was removed by your editor, but it would need to remain with any changes.
| import _csv | ||
| import csv | ||
| import sys | ||
| from typing import ( |
There was a problem hiding this comment.
Like with the header, I suspect this changes were done automatically by your editor. If not, can you explain your reasoning for the edits?
| open_mode: str = "wb" | ||
| if sys.version_info >= (3, 0): | ||
| open_mode: str = "w" | ||
| open_mode = "w" |
There was a problem hiding this comment.
I'd suggest simplifying even more:
with open(file. "w") as csvfile:
If we do make this change in this generator, I think it'd make sense to do the same across the repo (e.g. here).
| quoting=csv.QUOTE_MINIMAL, | ||
| lineterminator='\n') | ||
|
|
||
| schema_writer = csv.writer(csvfile, delimiter=',', quoting=csv.QUOTE_MINIMAL, lineterminator='\n') |
There was a problem hiding this comment.
Why remove the type hint, _csv._writer?
| field_set: str = 'base' | ||
| else: | ||
| field_set: str = key_parts[0] | ||
| key_parts = field['flat_name'].split('.') |
There was a problem hiding this comment.
Again, why remove the type hints?
|
This PR is stale because it has been open for 60 days with no activity. |
|
This PR has been automatically closed due to inactivity. If you'd like to Thank you for your contribution! |
2 participants
Changes in the
csv_generator.pyfile'w'instead of'wb'when running Python 3 or above. This ensures compatibility with both Python 2 and Python 3.str.lower()method is unnecessary when callingstr()on a boolean value. It has been removed for simplicity.