Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YAML vs TOML recorder format #615

Closed
beliaev-maksim opened this issue Dec 22, 2022 · 5 comments · Fixed by #624
Closed

YAML vs TOML recorder format #615

beliaev-maksim opened this issue Dec 22, 2022 · 5 comments · Fixed by #624
Assignees

Comments

@beliaev-maksim
Copy link
Collaborator

considering expansion of recorder to include matchers (I am currently working on it) I would like to reopen the discussion about recorded format

initially in #545 implementation was done via yaml. However, following thread in #545 (comment) we decided to go for TOML.

I would like to understand where we will face these issues with ambiguity in YAML.
The reason is following, TOML becomes completely unreadable for nested structures, compare YAML output below to following TOML.

@markstory
my proposal, make dumper exposed in user interface, allowing to switch between JSON/YAML/TOML/etc (considering that our data is serializable, and we use only built-in types: Dict, List, Str, Bool, Int, so, we should be serializable)
and we switch default output to YAML

responses:
  - response:
      method: GET
      url: 'http://localhost:37677/404'
      body: 404 Not Found
      status: 404
      content_type: text/plain
      auto_calculate_content_length: false
      matchers:
        - query_param_matcher:
            matcher_import_path: responses.matchers
            args:
              strict_match: true
              params: {}
  - response:
      method: GET
      url: 'http://localhost:37677/status/wrong'
      body: Invalid status code
      status: 400
      content_type: text/plain
      auto_calculate_content_length: false
      matchers:
        - query_param_matcher:
            matcher_import_path: responses.matchers
            args:
              strict_match: true
              params: {}
  - response:
      method: GET
      url: 'http://localhost:37677/500?query=smth'
      body: 500 Internal Server Error
      status: 500
      content_type: text/plain
      auto_calculate_content_length: false
      matchers:
        - query_param_matcher:
            matcher_import_path: responses.matchers
            args:
              strict_match: true
              params:
                query: smth
        - query_string_matcher:
            matcher_import_path: responses.matchers
            args:
              query: query=smth
  - response:
      method: PUT
      url: 'http://localhost:37677/202'
      body: OK
      status: 202
      content_type: text/plain
      auto_calculate_content_length: false
      matchers:
        - query_param_matcher:
            matcher_import_path: responses.matchers
            args:
              strict_match: true
              params: {}
[[responses]]

[responses.response]
method = "GET"
url = "http://localhost:37677/404"
body = "404 Not Found"
status = 404
content_type = "text/plain"
auto_calculate_content_length = false

[[responses.response.matchers]]

[responses.response.matchers.query_param_matcher]
matcher_import_path = "responses.matchers"

[responses.response.matchers.query_param_matcher.args]
strict_match = true

[responses.response.matchers.query_param_matcher.args.params]

[[responses]]

[responses.response]
method = "GET"
url = "http://localhost:37677/status/wrong"
body = "Invalid status code"
status = 400
content_type = "text/plain"
auto_calculate_content_length = false

[[responses.response.matchers]]

[responses.response.matchers.query_param_matcher]
matcher_import_path = "responses.matchers"

[responses.response.matchers.query_param_matcher.args]
strict_match = true

[responses.response.matchers.query_param_matcher.args.params]

[[responses]]

[responses.response]
method = "GET"
url = "http://localhost:37677/500?query=smth"
body = "500 Internal Server Error"
status = 500
content_type = "text/plain"
auto_calculate_content_length = false

[[responses.response.matchers]]

[responses.response.matchers.query_param_matcher]
matcher_import_path = "responses.matchers"

[responses.response.matchers.query_param_matcher.args]
strict_match = true

[responses.response.matchers.query_param_matcher.args.params]
query = "smth"

[[responses.response.matchers]]

[responses.response.matchers.query_string_matcher]
matcher_import_path = "responses.matchers"

[responses.response.matchers.query_string_matcher.args]
query = "query=smth"

[[responses]]

[responses.response]
method = "PUT"
url = "http://localhost:37677/202"
body = "OK"
status = 202
content_type = "text/plain"
auto_calculate_content_length = false

[[responses.response.matchers]]

[responses.response.matchers.query_param_matcher]
matcher_import_path = "responses.matchers"

[responses.response.matchers.query_param_matcher.args]
strict_match = true

[responses.response.matchers.query_param_matcher.args.params]
@hubertdeng123
Copy link
Member

I'm going to remove the Untriaged label here since it doesn't seem actionable immediately. Also, seems like @beliaev-maksim is a pretty big contributor here.

@markstory
Copy link
Member

markstory commented Jan 6, 2023

my proposal, make dumper exposed in user interface, allowing to switch between JSON/YAML/TOML/etc (considering that our data is serializable, and we use only built-in types: Dict, List, Str, Bool, Int, so, we should be serializable)
and we switch default output to YAML

Having the dumper be swappable makes sense to me. Should we be clever and have a mapping of file extensions to dump loaders? This would let userland code continue to use toml if they have those files and switching to yaml is as simple as changing the file extension for their dump files.

@beliaev-maksim
Copy link
Collaborator Author

@markstory
I am not 100% sure. We can implement the map, however, then we need to support all the formats by ourselves and that means have additional dependencies in the project.
What I had in mind is that we set only YAML dependency in responses. And we expose dump method to users, so, then users will be able to set any other dependency (eg toml/tomli/etc) in their project and do dump/read by themselves.

Otherwise, I am afraid, we might go on a slippery way.

@markstory
Copy link
Member

What I had in mind is that we set only YAML dependency in responses. And we expose dump method to users, so, then users will be able to set any other dependency (eg toml/tomli/etc) in their project and do dump/read by themselves.

Ok. I was thinking that the map would only contain toml and yaml. Any additional formats would need to be registered/added by userland code. I agree that we don't want to support all the formats possible as that would be too many dependencies.

@dwt
Copy link

dwt commented Feb 3, 2023

I'm biased, I like yaml a lot more than toml, and not just because bugs like this make toml unwieldy.

Still alternative serializers seem like great idea to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants