feat: redesign validations #105

mallachari · 2021-02-15T02:50:09Z

Makes validations being displayed in more human friendly way.

marcelltoth

Looking good, great work Jakub!

The hard blockers are "match pattern value" because it's a functional non-compliance (unless @philsturgeon says it's OK), and the potential key duplication bug, but that's trivial to fix.

marcelltoth · 2021-02-15T09:37:56Z

src/components/shared/Validations.tsx

+  maxLength: value => `<= ${value} characters`,
+};
+
+const validationsFormatter = (name: string) => (value: unknown[] | unknown): ValidationFormat | null => {


An unknown[] | unknown is still unknown, although I can see your point that it increases clarity somewhat.

We shouldn't be using this many unknowns to begin with, but that's not your fault, that's how validations are defined unfortunately.

Also I'd consider calling the function createValidationsFormatter as it's a HOF. It isn't the formatter itself, rather it creates the function that will be the formatter.

marcelltoth · 2021-02-15T09:40:15Z

src/components/shared/Validations.tsx

+  maxItems: value => `<= ${value} items`,
+  maxLength: value => `<= ${value} characters`,


Nice catch, my bad!

marcelltoth · 2021-02-15T09:41:19Z

src/components/shared/Validations.tsx

-    );
-  }
-  const validation = Array.isArray(value) ? value : [value];
+const KeyValueValidation = ({ className, name, values }: { className?: string; name: string; values: string[] }) => {


This function is much clearer now, great job!

marcelltoth · 2021-02-15T09:43:35Z

src/components/shared/Validations.tsx

+      {values.map(value => (
+        <Text key={value} ml={2} px={1} fontFamily="mono" border rounded="lg" className={className}>
+          {value}
+        </Text>
+      ))}


Are you sure values are unique? I know that's how things generally are, but I like to be very cautious with keys, because they cause hard-to-debug undefined behavior when non-unique. Please either run it through uniq or use another key, such as the index. Whichever you think fits the business requirements better.

marcelltoth · 2021-02-15T09:46:48Z

src/components/shared/__tests__/Validations.spec.tsx

+    expect(wrapper).toIncludeText('>= 10<= 40');
+    expect(wrapper).toIncludeText('Allowed values:10203040');
+    expect(wrapper).toIncludeText('Default value:20');
+    expect(wrapper).toIncludeText('Multiple of value:10');
+    expect(wrapper).toIncludeText('Example value:20');


I see you're attempting to use the good approach even with the not-so-good framework. We can likely discuss introduction of RTL in here later. Looks good for now.

marcelltoth · 2021-02-15T09:50:03Z

src/components/shared/Validations.tsx

+  multipleOf: validationsFormatter('Multiple of'),
+  pattern: validationsFormatter('Match pattern'),


Your abstraction falls a bit short here. These two shouldn't say values.

I'm willing to let it slide for now though if @philsturgeon is, too.

cc @philsturgeon Is it acceptable that it currently says "Match pattern value" and "Multiple of value"?

Oh yeah, right, it shouldn't. I'll think of better way

P0lip · 2021-02-15T12:36:47Z

src/components/shared/__tests__/Validations.spec.tsx

+    const validations = getValidationsFromSchema(node);
+    const wrapper = mount(<Validations validations={validations} />);
+
+    expect(wrapper).toIncludeText('>= 10<= 40');


You could probably just do

Suggested change

expect(wrapper).toIncludeText('>= 10<= 40');

expect(wrapper.text()).toMatchInlineSnapshot(

and verify the output

Do we like snaphosts? 😉

Well, I don't like them, BUT inline snapshots are okay.

P0lip · 2021-02-15T12:40:59Z

src/components/shared/Validations.tsx

 }
+
+function stringifyValue(value: unknown) {
+  return typeof value === 'string' ? `"${value}"` : String(value);


Any reason not to use JSON.stringify?

Right, might be better

P0lip · 2021-02-15T12:42:11Z

src/components/shared/Validations.tsx

+  enum: validationsFormatter('Allowed'),
+  examples: validationsFormatter('Example'),
+  example: validationsFormatter('Example'),
+  ['x-example']: validationsFormatter('Example'),


I'm not sure if json-schema-tree supports this. You might want to verify it.

It might not. Same with const so I took it from fragment to not update JST but we might want to do that. Though not sure if x-example is worth it.

so I took it from fragment to not update JST

We shouldn't be using fragment. 😬

mallachari · 2021-02-15T15:06:35Z

@philsturgeon do you have any remarks from your point of view?

P0lip · 2021-02-15T15:23:18Z

src/components/shared/Validations.tsx

+          ...(schemaNode.annotations.examples ? { examples: schemaNode.annotations.examples } : null),
+        }
+      : null),
+    ...('fragment' in schemaNode


We shouldn't be using fragment.
You can leave a todo note here (or even better create an issue in jst) and I'll get it done soon.

Alright, I'll put a note here and create issue for JST

JST bumped and fragment usage removed

philsturgeon

Not sure if these comments matter but I noticed some invalid OAS. I’ll review on computer soon but I’m sick and dying today so still try and look again when I can brain.

src/__fixtures__/default-schema.json

stoplight-bot · 2021-06-24T17:10:02Z

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: redesign validations

819957b

mallachari requested review from a team and philsturgeon February 15, 2021 02:50

marcelltoth suggested changes Feb 15, 2021

View reviewed changes

Jakub Jankowski added 2 commits February 15, 2021 13:39

fix: formatters

910c72b

Merge branch 'beta' into feat/validations

37b219f

P0lip reviewed Feb 15, 2021

View reviewed changes

chore: use JSON.stringify

a59d0b9

mallachari force-pushed the feat/validations branch from b774989 to a59d0b9 Compare February 15, 2021 13:00

mallachari requested a review from marcelltoth February 15, 2021 13:01

feat: support x-example

b437a72

marcelltoth approved these changes Feb 15, 2021

View reviewed changes

mallachari requested a review from P0lip February 15, 2021 15:04

P0lip reviewed Feb 15, 2021

View reviewed changes

chore: comments

6a0b1b7

philsturgeon reviewed Feb 15, 2021

View reviewed changes

src/__fixtures__/default-schema.json Outdated Show resolved Hide resolved

src/__fixtures__/default-schema.json Outdated Show resolved Hide resolved

fix: fixtures

6a57473

mallachari force-pushed the feat/validations branch from 28db2b4 to 6a57473 Compare February 15, 2021 15:54

mallachari mentioned this pull request Feb 15, 2021

feat: extend annotations stoplightio/json-schema-tree#4

Merged

feat: bump JST and use annotations

d4a0af5

mallachari requested a review from P0lip February 16, 2021 09:28

P0lip approved these changes Feb 16, 2021

View reviewed changes

mallachari merged commit 42ff7a4 into beta Feb 16, 2021

mallachari deleted the feat/validations branch February 16, 2021 10:23

philsturgeon mentioned this pull request Jun 21, 2021

Release v4.0.0 stable #152

Closed

stoplight-bot added the released label Jun 24, 2021

		maxItems: value => `<= ${value} items`,
		maxLength: value => `<= ${value} characters`,

		multipleOf: validationsFormatter('Multiple of'),
		pattern: validationsFormatter('Match pattern'),

	expect(wrapper).toIncludeText('>= 10<= 40');
	expect(wrapper.text()).toMatchInlineSnapshot(

feat: redesign validations #105

feat: redesign validations #105

Uh oh!

Conversation

mallachari commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcelltoth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mallachari commented Feb 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philsturgeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stoplight-bot commented Jun 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mallachari commented Feb 15, 2021 •

edited

Loading