feat: Add new client parameters: encoding #330

dongfangtianyu · 2021-02-08T13:39:21Z

If there are non-ASCII characters in openapi (eg: https://v1.api.tttt.one/openapi.json)
Garbled characters may appear in the generated client file.

To this end, add encoding parameter to cli，Similar problems can be ended by --encoding=utf-8

codecov · 2021-02-08T13:50:09Z

Codecov Report

Merging #330 (e0a1392) into main (2d305dd) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #330   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1398      1410   +12     
=========================================
+ Hits          1398      1410   +12

Impacted Files	Coverage Δ
openapi_python_client/__init__.py	`100.00% <100.00%> (ø)`
openapi_python_client/cli.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d305dd...e0a1392. Read the comment docs.

emann

Thanks @dongfangtianyu! One small nitpick - I think using file_encoding everywhere instead of encoding is a bit clearer.

While I think this is still a useful CLI option, maybe utf-8 should be our default encoding anyways? What do you think @dbanty

emann · 2021-02-08T16:46:20Z

openapi_python_client/cli.py

@@ -116,6 +116,7 @@ def generate(
    url: Optional[str] = typer.Option(None, help="A URL to read the JSON from"),
    path: Optional[pathlib.Path] = typer.Option(None, help="A path to the JSON file"),
    custom_template_path: Optional[pathlib.Path] = typer.Option(None, **custom_template_path_options),  # type: ignore
+    encoding: Optional[str] = typer.Option(None, help="Set file encoding for client files"),  # type: ignore


Suggested change

encoding: Optional[str] = typer.Option(None, help="Set file encoding for client files"), # type: ignore

encoding: Optional[str] = typer.Option(None, help="Encoding used when writing generated files"), # type: ignore

emann · 2021-02-08T16:54:21Z

openapi_python_client/cli.py

@@ -116,6 +116,7 @@ def generate(
    url: Optional[str] = typer.Option(None, help="A URL to read the JSON from"),
    path: Optional[pathlib.Path] = typer.Option(None, help="A path to the JSON file"),
    custom_template_path: Optional[pathlib.Path] = typer.Option(None, **custom_template_path_options),  # type: ignore
+    encoding: Optional[str] = typer.Option(None, help="Set file encoding for client files"),  # type: ignore


Using an enum of choices would be better than an arbitrary string here... although unless someone has created an enum with every encoding we'll have to manually add supported encoding options. Not the end of the world IMO + gives us a bit more control over what we officially support.

dbanty · 2021-02-08T17:17:41Z

maybe utf-8 should be our default encoding anyways?

Agreed.

Using an enum of choices would be better than an arbitrary string here

I don't think there is any built in enum with available encodings, and it's probably possible to register different codecs somehow. I don't think we want to be in the business of trying to maintain or restrict a list, we support whatever Python supports.

We could, however, add some validation here using something like codecs.getencoder to provide a useful error message to the user instead of just crashing when writing if the encoding is not supported.

dongfangtianyu · 2021-02-09T08:31:28Z

In general, we only use UTF-8, but we don't know whether users will use other codes，

if encoding is not specified the encoding used is platform dependent

May user want the operating system to decide codecs, need pass a None to encoding,

What bothers me is that I haven't found a way to use the default value to gracefully deliver None

Maybe my idea is wrong, user need not use the None

dbanty · 2021-02-09T15:32:25Z

May user want the operating system to decide codecs, need pass a None to encoding,

I think the more obvious behavior to the user is to explicitly set an encoding default (UTF-8). I can't think of a scenario where having the system decide the codec is sustainable or reproducible. At least for me, I tend to generate clients in CI after testing locally. If CI and local had different system codecs it would be failing without me knowing why which could be frustrating.

dongfangtianyu · 2021-02-15T00:00:00Z

Hi, @dbanty , I know where our differences are.

Now, my way of using it is to move the generated folder directly into the existing test project.
So I want the client's file code to be consistent with the original file code of the test project -- determined by the operating system

Is the right way to use it is to think of the client as a separate Python package? Only import and call, should not be modified
If so, then UTF-8 is very appropriate.

dongfangtianyu · 2021-02-15T00:00:20Z

Hi, @dbanty , I know where our differences are.

Now, my way of using it is to move the generated folder directly into the existing test project.
So I want the client's file code to be consistent with the original file code of the test project -- determined by the operating system

Is the right way to use it is to think of the client as a separate Python package? Only import and call, should not be modified
If so, then UTF-8 is very appropriate.

emann

Just one more nitpick and then this will be good to go!

emann · 2021-02-15T15:13:50Z

openapi_python_client/__init__.py

@@ -44,10 +44,19 @@ class Project:
    project_name_override: Optional[str] = None
    package_name_override: Optional[str] = None
    package_version_override: Optional[str] = None
-
-    def __init__(self, *, openapi: GeneratorData, meta: MetaType, custom_template_path: Optional[Path] = None) -> None:
+    file_encoding: str


Suggested change

file_encoding: str

Don't need this at the class level

emann · 2021-02-15T15:14:18Z

openapi_python_client/__init__.py

        self.openapi: GeneratorData = openapi
        self.meta: MetaType = meta
+        self.encoding = file_encoding


This should be self.file_encoding everywhere

emann · 2021-02-15T15:22:03Z

@dongfangtianyu there isn't necessarily a right or wrong way to use the package, however if I had to guess I would think most people use the client as a standalone python package and don't edit it much. That being said, obviously you are free to use it how you please and we aim to support all ways of using the project.

I do think utf-8 encoding makes sense as a default - it is incredibly common and will work in 99% of use cases + fixes issues such as yours without any consequence for users who don't explicitly need utf-8. Worst come to worst a user can specify their own encoding if necessary thanks to this PR you've made!

… default is UTF-8

CHANGELOG.md

dongfangtianyu force-pushed the cli branch from a683c06 to 3563804 Compare February 8, 2021 13:48

dongfangtianyu force-pushed the cli branch from 3563804 to d162058 Compare February 8, 2021 14:11

emann requested changes Feb 8, 2021

View reviewed changes

emann requested changes Feb 15, 2021

View reviewed changes

dongfangtianyu added 5 commits February 16, 2021 18:21

feat: Add new client parameters: encoding

3e7732f

fix: Modify the parameter name encoding to file_encoding, and set the…

1fd0ced

… default is UTF-8

fix: test coverage (openapi_python_client/cli.py)

e1dbdc5

fix: Modify the parameter name encoding to file_encoding,

c37268b

docs: Add CHANGELOG entry for openapi-generators#330

41af1f9

dongfangtianyu force-pushed the cli branch from 8d61c1d to 41af1f9 Compare February 16, 2021 10:32

emann reviewed Feb 16, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

emann added 3 commits February 16, 2021 14:54

Update CHANGELOG.md

eebcc2b

Merge branch 'main' into cli

62e414b

Merge branch 'main' into cli

e0a1392

emann enabled auto-merge (squash) February 16, 2021 20:01

emann approved these changes Feb 16, 2021

View reviewed changes

emann merged commit 7f8d10e into openapi-generators:main Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add new client parameters: encoding #330

feat: Add new client parameters: encoding #330

dongfangtianyu commented Feb 8, 2021 •

edited

Loading

codecov bot commented Feb 8, 2021 •

edited

Loading

emann left a comment

emann Feb 8, 2021

emann Feb 8, 2021

dbanty commented Feb 8, 2021

dongfangtianyu commented Feb 9, 2021

dbanty commented Feb 9, 2021

dongfangtianyu commented Feb 15, 2021

dongfangtianyu commented Feb 15, 2021

emann left a comment

emann Feb 15, 2021

emann Feb 15, 2021

emann commented Feb 15, 2021

	encoding: Optional[str] = typer.Option(None, help="Set file encoding for client files"), # type: ignore
	encoding: Optional[str] = typer.Option(None, help="Encoding used when writing generated files"), # type: ignore

feat: Add new client parameters: encoding #330

feat: Add new client parameters: encoding #330

Conversation

dongfangtianyu commented Feb 8, 2021 • edited Loading

codecov bot commented Feb 8, 2021 • edited Loading

Codecov Report

emann left a comment

Choose a reason for hiding this comment

emann Feb 8, 2021

Choose a reason for hiding this comment

emann Feb 8, 2021

Choose a reason for hiding this comment

dbanty commented Feb 8, 2021

dongfangtianyu commented Feb 9, 2021

dbanty commented Feb 9, 2021

dongfangtianyu commented Feb 15, 2021

dongfangtianyu commented Feb 15, 2021

emann left a comment

Choose a reason for hiding this comment

emann Feb 15, 2021

Choose a reason for hiding this comment

emann Feb 15, 2021

Choose a reason for hiding this comment

emann commented Feb 15, 2021

dongfangtianyu commented Feb 8, 2021 •

edited

Loading

codecov bot commented Feb 8, 2021 •

edited

Loading