Skip to content

Conversation

@vankop
Copy link
Member

@vankop vankop commented Sep 28, 2019

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

#42
2nd proposal

Breaking Changes

no

Additional Info

Looks like for array and object we can skip formatting type.

@codecov
Copy link

codecov bot commented Sep 28, 2019

Codecov Report

Merging #59 (db7e4f6) into master (62fb107) will decrease coverage by 0.58%.
The diff coverage is 97.67%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #59 +/- ## ========================================== - Coverage 98.72% 98.13% -0.59%  ========================================== Files 5 5 Lines 550 645 +95 Branches 250 268 +18 ========================================== + Hits 543 633 +90  - Misses 7 12 +5 
Impacted Files Coverage Δ
src/ValidationError.js 97.68% <97.33%> (-0.67%) ⬇️
src/index.js 100.00% <100.00%> (ø)
src/keywords/absolutePath.js 100.00% <100.00%> (ø)
src/util/Range.js 100.00% <100.00%> (ø)
src/validate.js 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 62fb107...db7e4f6. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Let's add test for multiple types, output should be like <key>: string | integer.

For objects we don't expand them, right?

@vankop
Copy link
Member Author

vankop commented Sep 28, 2019

Yes objects and arrays are not expanded

@vankop
Copy link
Member Author

vankop commented Sep 28, 2019

Multiple types could be only with anyOf | oneOf. Here we just look on type if it exists

@vankop
Copy link
Member Author

vankop commented Sep 28, 2019

so @evilebottnawi what we need in this PR still? Tests on additionalProperties ?

@alexander-akait
Copy link
Member

alexander-akait commented Sep 28, 2019

@vankop all is good, i will review deeply this in near future

@alexander-akait
Copy link
Member

/cc @vankop need rebase, also i think we should improve output using \n (like prettier do with objects 😄 ), because some error is very long

@vankop
Copy link
Member Author

vankop commented Nov 11, 2019

also i think we should improve output using \n

@evilebottnawi you have any ideas how to do it better?

When I did this I was thinking about it, but did not realized how to do it better, since it depends totally on terminal window size + font type/size. Approach when we rely only on amount of properties also fails because of glyph sizes

@alexander-akait
Copy link
Member

alexander-akait commented Nov 12, 2019

@vankop the good question, maybe we can solve this in other PR, i think packages like table have algorithm for this, need look on them logic

@vankop
Copy link
Member Author

vankop commented Nov 26, 2019

/cc @evilebottnawi

Ready to review

maybe we can solve this in other PR, i think packages like table have algorithm for this, need look on them logic

I think this really important, I will take a look in table package, thanks for suggestion

@vankop
Copy link
Member Author

vankop commented Nov 26, 2019

Also interesting question is - do we need sort properties alphabetically?

@alexander-akait
Copy link
Member

@vankop it is very old 😄 what we will do with it? close or rebase? 😄

@vankop
Copy link
Member Author

vankop commented Apr 15, 2023

I could merge main to make this relevant again

@alexander-akait
Copy link
Member

@vankop Yeah, let's do it

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

Labels

None yet

3 participants