-
Notifications
You must be signed in to change notification settings - Fork 13
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
lists in result messages #129
Conversation
🦋 Changeset detectedLatest commit: f732f93 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
src/validation-engine.js
Outdated
const items = Array.from(take(3, list)) | ||
return items.join(', ') + (items.length === 3 ? ' ...' : '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be useful to give even more info than just an ellipsis. If it does not affect performance too much
const items = Array.from(take(3, list)) | |
return items.join(', ') + (items.length === 3 ? ' ...' : '') | |
const { head, count } = take(3, list) | |
const items = Array.from(head) | |
return items.join(', ') + (items.length === 3 ? ` (and ${count} more)` : '') |
test/validation_message_test.js
Outdated
|
||
assert.strictEqual(report.results.length, 1) | ||
assert.strictEqual(report.results[0].message.length, 1) | ||
assert.strictEqual(report.results[0].message[0].value, 'Value is not one of the allowed values: a, b, c ...') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the message look like when items are IRIs?
added a commit to also address #131 because the information in the |
@giacomociti FYI this PR may have a regression which occurs for some cubes while not for others - can you reproduce? repro:
msg:
nb: some more repro cases
|
Nice catch @Rdataflow : the list of values allowed by I managed to fix the issue, also introducing a non-regression test. But I'm not super-happy with the design: the list initialization is now within the |
@giacomociti thanks for your fix - confirm it works here 👍 |
Not without diving deeper. I suggest we create a ticket to improve in the future and roll with what we have now |
should address #109 . Result messages include the first (three) elements of lists