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

Initial Validator #1

Merged
merged 7 commits into from
Nov 5, 2020
Merged

Initial Validator #1

merged 7 commits into from
Nov 5, 2020

Conversation

pinussilvestrus
Copy link
Contributor

@pinussilvestrus pinussilvestrus commented Nov 4, 2020

Related to camunda/element-templates-json-schema#11

Which issue does this PR address?

Related to camunda/element-templates-json-schema#11

Acceptance Criteria

  • Corresponds to the concept
  • Corresponds to the design

Definition of Done

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Nov 4, 2020
@pinussilvestrus pinussilvestrus marked this pull request as ready for review November 5, 2020 07:42
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Nov 5, 2020
.gitignore Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Added two comments, other than that 👍

@pinussilvestrus
Copy link
Contributor Author

I'll update the project to make it even more generic and not element-template specific.

@pinussilvestrus pinussilvestrus changed the title Initial Validator [WIP] Initial Validator Nov 5, 2020
@pinussilvestrus
Copy link
Contributor Author

pinussilvestrus commented Nov 5, 2020

I'd even go for

Validator#validate
Validator#validateAll

Instead of

Validator#validateTemplate
Validator#validateTemplates

@pinussilvestrus pinussilvestrus changed the title [WIP] Initial Validator Initial Validator Nov 5, 2020
@nikku
Copy link
Member

nikku commented Nov 5, 2020

I've pushed a few commits on top:

  • Using named exports, rather than default exports as these better operate with CJS / ESM
  • Unifying the validator functions so that both return { valid: true|false } (3fc8232)
  • Providing the validated object with the validation result, i.e. it changed to { valid, object, errors } (9eafa28)

The last two things are up for debate. I personally believe whoever uses the library will do his / her filtering / collecting of errors anyway, so the more generic API would work for me, too.

Added a test case, how extracting of valid templates looks: https://github.com/bpmn-io/json-schema-validator/pull/1/files#diff-8f573ab0dcdbf810d70135136d063a35a616b34aafaa78566ba814952eb1dc89R66.

Note, as mentioned, that an app likely does more. It will likely collect both valid templates and errors at the same time.

@pinussilvestrus
Copy link
Contributor Author

I like the more generic approach. Let's make the most out of it 👍

@pinussilvestrus pinussilvestrus merged commit 9d021a6 into main Nov 5, 2020
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Nov 5, 2020
@fake-join fake-join bot deleted the next branch November 5, 2020 10:42
@nikku
Copy link
Member

nikku commented Nov 5, 2020

I guess this one is ready to be released?

@pinussilvestrus
Copy link
Contributor Author

Yes! I plan to do the initial release for it today 👍

@pinussilvestrus
Copy link
Contributor Author

pinussilvestrus commented Nov 5, 2020

I would assume the API is stable, so v1.0.0 would be appropriate? What do you think?

@nikku
Copy link
Member

nikku commented Nov 5, 2020

Yes. Starting with a 0.0.1 works, too.

@nikku
Copy link
Member

nikku commented Nov 5, 2020

I'd start with 0.x, to be fair.

@pinussilvestrus
Copy link
Contributor Author

Yeah, let's keep the stableness open.

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

Successfully merging this pull request may close these issues.

None yet

2 participants