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

Add 'number' as a body value type #46

Closed

Conversation

ariel-m-s
Copy link
Contributor

Feature: Add 'number' as a body value type

Description

JSON does not define 'integer' and 'float' as valid data types, but rather merges them both into the 'number' data type. This PR includes 'number' as part of the data types, but does not remove 'integer' and 'float' for backward compatibility.

I think that 'array' and 'object' should also be allowed (somehow).

Requirements

None.

Additional changes

None.

@codecov-io
Copy link

Codecov Report

Merging #46 (32a0858) into master (b6ff279) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #46   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines          200       200           
=========================================
  Hits           200       200           
Impacted Files Coverage Δ
zum/requests/helpers.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 b6ff279...32a0858. Read the comment docs.

@ariel-m-s ariel-m-s marked this pull request as draft April 7, 2021 22:32
@ariel-m-s ariel-m-s marked this pull request as ready for review April 7, 2021 22:34
@daleal
Copy link
Owner

daleal commented Apr 7, 2021

Hi @ariel-m-s! When writing the description spec, I considered adding the number type, but I decided not to because it was completely redundant (in my opinion, the JSON format is wrong regarding the the merge of both types). Given that you can achieve the same result by using float, I still don't think it's a good idea to add that new type.
Regarding object and array, I don't think I'll add them either. For more complex structures, I think that allowing to pass a JSON file (or TOML or YAML, hopefully at some point) directly to the CLI is the better alternative. @sehnsucht13 was tackling that particular feature on #43, but I haven't heard from him for a while. I really haven't had the time to tackle that feature myself, but I'll wait for a while to see what @sehnsucht13 comes up with and, if he decides that he doesn't want to complete the feature, I'll add it on a PR for 0.3.0.

With all that being said, I'm going to close this Pull Request, as the type that you want to add is redundant.

@daleal daleal closed this Apr 7, 2021
@daleal daleal self-requested a review April 7, 2021 22:42
@daleal daleal added feature New feature or request wontfix This will not be worked on labels Apr 7, 2021
@ariel-m-s ariel-m-s deleted the feat/add-number-to-body-value-types branch April 7, 2021 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants