Skip to content

feat: do not wrap string validations #162

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 24, 2021
Merged

feat: do not wrap string validations #162

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
833151a
feat: do not wrap string validations
Aug 13, 2021
b632ec7
Merge branch 'master' into feat/string-validations-no-wrap
mnaumanali94 Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 28 additions & 28 deletions src/__tests__/__snapshots__/index.spec.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,8 +349,8 @@ exports[`HTML Output given anyOf combiner placed next to allOf given allOf mergi
</div>
<div>
<span>Allowed values:</span>
<span>\\"admin\\"</span>
<span>\\"editor\\"</span>
<span>admin</span>
<span>editor</span>
</div>
</div>
<div></div>
Expand Down Expand Up @@ -562,8 +562,8 @@ exports[`HTML Output given oneOf combiner placed next to allOf given allOf mergi
</div>
<div>
<span>Allowed values:</span>
<span>\\"admin\\"</span>
<span>\\"editor\\"</span>
<span>admin</span>
<span>editor</span>
</div>
</div>
<div></div>
Expand Down Expand Up @@ -1006,12 +1006,12 @@ exports[`HTML Output should match arrays/of-complex-objects.json 1`] = `
</div>
<div>
<span>Allowed value:</span>
<span>\\"Constant name\\"</span>
<span>Constant name</span>
</div>
<div>
<span>Example values:</span>
<span>\\"Example name\\"</span>
<span>\\"Different name\\"</span>
<span>Example name</span>
<span>Different name</span>
</div>
</div>
<div></div>
Expand Down Expand Up @@ -1126,12 +1126,12 @@ exports[`HTML Output should match arrays/of-complex-objects.json 1`] = `
<div><span>&gt;= 2 characters</span></div>
<div>
<span>Default value:</span>
<span>\\"[email protected]\\"</span>
<span>[email protected]</span>
</div>
<div>
<span>Example values:</span>
<span>\\"[email protected]\\"</span>
<span>\\"[email protected]\\"</span>
<span>[email protected]</span>
<span>[email protected]</span>
</div>
</div>
<div></div>
Expand Down Expand Up @@ -2461,9 +2461,9 @@ exports[`HTML Output should match combiners/allOfs/with-type.json 1`] = `
</div>
<div>
<span>Allowed values:</span>
<span>\\"Cancel\\"</span>
<span>\\"Confirm\\"</span>
<span>\\"Update\\"</span>
<span>Cancel</span>
<span>Confirm</span>
<span>Update</span>
</div>
</div>
<div></div>
Expand Down Expand Up @@ -2589,7 +2589,7 @@ exports[`HTML Output should match combiners/oneof-with-array-type.json 1`] = `
</div>
<div>
<span>Allowed value:</span>
<span>\\"test\\"</span>
<span>test</span>
</div>
</div>
<div></div>
Expand Down Expand Up @@ -2644,7 +2644,7 @@ exports[`HTML Output should match combiners/oneof-within-array-item.json 1`] = `
</div>
<div>
<span>Allowed value:</span>
<span>\\"test\\"</span>
<span>test</span>
</div>
</div>
<div></div>
Expand Down Expand Up @@ -2687,12 +2687,12 @@ exports[`HTML Output should match default-schema.json 1`] = `
</div>
<div>
<span>Allowed value:</span>
<span>\\"Constant name\\"</span>
<span>Constant name</span>
</div>
<div>
<span>Example values:</span>
<span>\\"Example name\\"</span>
<span>\\"Different name\\"</span>
<span>Example name</span>
<span>Different name</span>
</div>
</div>
<div></div>
Expand Down Expand Up @@ -2807,12 +2807,12 @@ exports[`HTML Output should match default-schema.json 1`] = `
<div><span>&gt;= 2 characters</span></div>
<div>
<span>Default value:</span>
<span>\\"[email protected]\\"</span>
<span>[email protected]</span>
</div>
<div>
<span>Example values:</span>
<span>\\"[email protected]\\"</span>
<span>\\"[email protected]\\"</span>
<span>[email protected]</span>
<span>[email protected]</span>
</div>
</div>
<div></div>
Expand Down Expand Up @@ -3439,12 +3439,12 @@ exports[`HTML Output should match tickets.schema.json 1`] = `
</div>
<div>
<span>Allowed values:</span>
<span>\\"COMPLETED\\"</span>
<span>\\"ACTIVE\\"</span>
<span>COMPLETED</span>
<span>ACTIVE</span>
</div>
<div>
<span>Default value:</span>
<span>\\"ACTIVE\\"</span>
<span>ACTIVE</span>
</div>
</div>
<div></div>
Expand Down Expand Up @@ -3529,10 +3529,10 @@ exports[`HTML Output should match tickets.schema.json 1`] = `
</div>
<div>
<span>Allowed values:</span>
<span>\\"HOMEPRINT\\"</span>
<span>\\"TICKETLESS\\"</span>
<span>\\"PRINT_AT_KIOSK\\"</span>
<span>\\"SECURE_PAPER\\"</span>
<span>HOMEPRINT</span>
<span>TICKETLESS</span>
<span>PRINT_AT_KIOSK</span>
<span>SECURE_PAPER</span>
</div>
</div>
<div></div>
Expand Down
6 changes: 3 additions & 3 deletions src/components/shared/Validations.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ const createValidationsFormatter = (name: string, options?: { exact?: boolean; n
};

const validationFormatters: Record<string, (value: unknown) => ValidationFormat | null> = {
enum: createValidationsFormatter('Allowed'),
examples: createValidationsFormatter('Example'),
enum: createValidationsFormatter('Allowed', { nowrap: true }),
Copy link
Contributor

@P0lip P0lip Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, and how will one recognize such an enum?

enum: [1, "1"]

I don't think it's a good decision to remove quotes. These are strings, so let them be.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't in such a case. That's why I added it in the first place.
@mnaumanali94 is it really needed to remove those quotes? We're losing a type then.

Copy link
Contributor

@billiegoose billiegoose Aug 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, and how will one recognize such an enum?

enum: [1, "1"]

This is a terrible enum! If someone is mixing stringified numbers and numberified numbers in an enum, their API is beyond our help. :trollface:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it is, but it's just to show the example.
The below cases are valid tho:

enum: ["1", "2", "3"]

enum: [1, 2, 3]

If we render them the same way, how will one be able to tell the difference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@P0lip are enums the only potential problem? Or can you think about more examples where this change would cause confusion? 🤔

What if we made the enums an exception here? @mnaumanali94 is a strong proponent of that change. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on the scenario.
Consider the below:

{ 
  "enum": ["0", 1, "2"]
}

or

{
  "type": ["string", "number"],
  "enum": ["0", 1, "2"]
}

One won't be able to tell the difference once we apply the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Have you ever seen an enum like this though? 🤦🏼

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should

type: string,
enum: [asc, desc]

In our docs, these values should appear as "asc", "desc" is what you're suggesting?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first example is quite common actually.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking this is not a common use case. The concern is primarily folks thinking "" are part of the value which would be wrong. Lets get this in, and see if there's a reaction from our consumers. If so, we can use colours to denote string vs integer values.

examples: createValidationsFormatter('Example', { nowrap: true }),
multipleOf: createValidationsFormatter('Multiple of', { exact: true }),
pattern: createValidationsFormatter('Match pattern', { exact: true, nowrap: true }),
default: createValidationsFormatter('Default'),
default: createValidationsFormatter('Default', { nowrap: true }),
};

const oasFormats = {
Expand Down
6 changes: 3 additions & 3 deletions src/components/shared/__tests__/Validations.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ describe('Validations component', () => {
const wrapper = mount(<Validations validations={validations} />);

expect(wrapper).toIncludeText('>= 2 characters<= 4 characters');
expect(wrapper).toIncludeText('Default value:"foo"');
expect(wrapper).toIncludeText('Example values:"Example 1""Example 2"');
expect(wrapper).toIncludeText('Allowed value:"bar"');
expect(wrapper).toIncludeText('Default value:foo');
expect(wrapper).toIncludeText('Example values:Example 1Example 2');
expect(wrapper).toIncludeText('Allowed value:bar');
});

it('should not render hidden example validations', () => {
Expand Down