opensearch-project / opensearch-py

Python Client for OpenSearch
https://opensearch.org/docs/latest/clients/python/
Apache License 2.0
338 stars 170 forks source link

[BUG] Missing or unnecessary utf-8 header in .py files #613

Closed dblock closed 10 months ago

dblock commented 10 months ago

What is the bug?

In #557 we added UTF-8 headers, but lost them in https://github.com/opensearch-project/opensearch-py/commit/bcfef113c43f74c41dec9c43ba54d6e00cc17c90 again. Do we need them?

What is the expected behavior?

Either have them or don't have them, and add a linter that ensures headers are present/not present in all files.

samuelorji commented 10 months ago

can i pick this up ?

jayaddison commented 10 months ago

Is there some background information about why coding declarations are required in this codebase? My understanding is that UTF-8 is the default for Python source files (ref).

dblock commented 10 months ago

@jayaddison You actually have a point, these might not be required at all. I think when I did https://github.com/opensearch-project/opensearch-py/pull/557 I was lazy and didn't actually check and added them because they were in some places. @samuelorji feel free to remove them if that's right. We should probably ensure we have tests that have some UTF-8 in them and that it's decoded properly.

I updated the issue title/description accordingly.

jayaddison commented 10 months ago

Thanks @dblock - yep, unless there is a specific reason to specify UTF-8 for forwards-looking reasons (not that I'd expect Python to change the default any time soon), or for some editor/environmental reason that requires it, I think fewer moving parts (in particular, not needing a lintcheck) could make sense.

I won't touch the code myself given that there's interest from others in it, though I'll try to remember to check back on this in a few weeks/months.

dblock commented 10 months ago

Looks like @samuelorji is picking this up, but don't let us stop you from contributing any time you can @jayaddison!

jayaddison commented 10 months ago

@dblock I think this can now be closed?