Comments / formatting #84

JBBianchi · 2021-05-14T10:22:26Z

What this PR does / why we need it:
Improves code quality and consistancy with linting and prettifying.
Adds comments & file headers.
Fixes builder problems.

Linked to #79
Linked to #94

Closes #89
Closes #90
Closes #91
Closes #92
Closes #93

- refactored WorkflowValidator.validationErrors as errors to keep consistancy with the underlying validator and other validators - added missing comments - fixed README `Add as dependency to your project`, used the scoped name - added imports in README examples - added 'validate parts of a workflow' example in README Signed-off-by: JBBianchi <jb.bianchi@neuroglia.io>

- added prettier & eslint - fixed eslint errors/warnings Signed-off-by: JBBianchi <jb.bianchi@neuroglia.io>

- formatted code with prettier Signed-off-by: JBBianchi <jb.bianchi@neuroglia.io>

- added husky for pre commit formatting/pre push test - fixed prettier commands - formatted code Signed-off-by: JBBianchi <jb.bianchi@neuroglia.io>

JBBianchi · 2021-05-14T10:41:09Z

Not sure husky/lint-staged work as expected...

JBBianchi · 2021-05-15T13:18:22Z

@antmendoza @tsurdilo are you ok with merging this ? I'm not a 100% sure about the husky/lint-staged config but it's a step forward anyways and shouldn't break anything :)

antmendoza · 2021-05-16T10:19:02Z

I will review it today, thank you @JBBianchi

antmendoza · 2021-05-16T19:30:44Z

Hi @JBBianchi

I think that husky is not working for me if I am getting it right.

The pre-commit hook will run "npx lint-staged", which run "npm run format" for all the staged files, isn't it?

In my case nothing happen

any suggestions ?

JBBianchi · 2021-05-16T19:43:19Z

Hi @JBBianchi

I think that husky is not working for me if I am getting it right.

The pre-commit hook will run "npx lint-staged", which run "npm run format" for all the staged files, isn't it?

In my case nothing happen

any suggestions ?

lint-staged will run "npm run format" if there is any .ts staged if I understood well but I must admit I don't think it's working.

- fixed builder for union types - rename "validatorFn" by "buildingFn" in builder(s) - fixed default kind/type for Subflowstate, Function, Eventdef - added file header - "installed" husky - updated lint-staged config Signed-off-by: JBBianchi <jb.bianchi@neuroglia.io>

JBBianchi · 2021-05-17T09:43:25Z

I fixed lint-staged/husky I think. 1st, I forgot to install husky properly, 2nd I didn't use the proper command to format (npm run format is formatting all the files, with lint-staged we want to only format the staged ones. So in the end, I just used prettier --write).

I also added file header and some fixes:
#89
#90
#91
#92
#93
#94

antmendoza

husky is not working for me, no idea why.

Instead it works adding the pre-commit hook like it shows the documentation here https://www.npmjs.com/package/husky
npx husky add .husky/pre-commit "npm test"

Could you check if this is working also for you?

I am gonna merge this anyway, we can fix it in another pr.

Thank you @JBBianchi

antmendoza · 2021-05-17T19:54:59Z

.github/ISSUE_TEMPLATE/bug-report.md

-
-**Anything else we need to know?**:
-
+---


also the md files are been formatting, cool !!!

prettier supports MD indeed but I don't really understand why they have been formatted... I thought I specified .ts only...

antmendoza · 2021-05-17T19:59:48Z

README.md

+import { validators, Specification } from '@severlessworkflow/sdk-typescript';
+
+const injectionState: Specification.Injectstate = workflow.states[0];
+const injectionStateValidator: ValidateFunction<Specification.Injectstate> = validators.get('Injectstate');


should we create a wrapper (like workflowValidator) to validate individual elements?

(not now, just wondering)

Functions, retries, and event can be defined in different files, so I think at least for those element we have to.

+ if I where a user of the sdk, I don't want to learn the keys for each element to retrieve the validator from validators

The 'keys' are the type names of Specification but I agree with you, it could be improved. I need to double check but I don't think we can use the type directly in TS like validator.get(Specification.Injectstate) because the type doens't exist at runtime. Anyhow, good idea, there is room for improvement here.

One side note though, more regarding the schema, I think Events/Retries/Functions cannot be used by themselves because they are based on custom properties that don't exist in the JSON Schema specification. So they can be "pointed at" but not used alone. I might be wrong though. (See little chat in the JSON Schema Slack)

antmendoza · 2021-05-17T20:09:31Z

tools/consts.ts

@@ -0,0 +1,21 @@
+export const fileHeader = `/*
+* Copyright 2021-Present The Serverless Workflow Specification Authors


antmendoza · 2021-05-17T20:23:27Z

.husky/.gitignore

@@ -0,0 +1 @@
+_


I am just curious, what this is supposing to do?

It's added by husky. When doing husky install, it creates .husky/ with that .gitignore and a _/ with a .sh in it.

JBBianchi · 2021-05-17T21:13:08Z

husky is not working for me, no idea why.

Instead it works adding the pre-commit hook like it shows the documentation here https://www.npmjs.com/package/husky
npx husky add .husky/pre-commit "npm test"

Could you check if this is working also for you?

I am gonna merge this anyway, we can fix it in another pr.

Thank you @JBBianchi

Maybe you need to run the husky install ? It creates .husky/_/ containing a husky.sh. I'll double check tomorrow, I'm not familiar with husky/lint-staged.

tsurdilo · 2021-05-17T21:32:51Z

is there anything I can help with this? if you have some steps i can follow can try on a clean box

JBBianchi · 2021-05-18T08:43:13Z

@tsurdilo I would be nice if you could get the project, run npm install, create a new dummy .ts file with something like export const foo = "bar"; and commit locally.

Huksy should kick in and, in the end, the file should be prettyfied as export const foo = 'bar'; (with single quotes)

JBBianchi · 2021-05-18T10:46:58Z

I think @antmendoza is right, they changed the way to handle hooks. The js/json/package method has been replaced by the .husky directory thing (https://blog.typicode.com/husky-git-hooks-javascript-config/). I'll fix that.

JBBianchi added 4 commits May 14, 2021 09:34

Prettier & ESLint

888fd98

- added prettier & eslint - fixed eslint errors/warnings Signed-off-by: JBBianchi <jb.bianchi@neuroglia.io>

Formatted

99e92e6

- formatted code with prettier Signed-off-by: JBBianchi <jb.bianchi@neuroglia.io>

Husky/Lint-Staged

4efa052

- added husky for pre commit formatting/pre push test - fixed prettier commands - formatted code Signed-off-by: JBBianchi <jb.bianchi@neuroglia.io>

JBBianchi requested review from antmendoza and tsurdilo as code owners May 14, 2021 10:22

JBBianchi mentioned this pull request May 15, 2021

Create pipeline to publish to npmjs #87

Closed

Builder & headers

5df3e28

- fixed builder for union types - rename "validatorFn" by "buildingFn" in builder(s) - fixed default kind/type for Subflowstate, Function, Eventdef - added file header - "installed" husky - updated lint-staged config Signed-off-by: JBBianchi <jb.bianchi@neuroglia.io>

antmendoza reviewed May 17, 2021

View reviewed changes

antmendoza merged commit 41a4e8a into serverlessworkflow:main May 17, 2021

This was referenced May 18, 2021

Initial data should be populated in builders, not in validators #94

Closed

Add better code formatting #79

Closed

JBBianchi deleted the comments-formatting branch May 18, 2021 09:09

JBBianchi mentioned this pull request May 19, 2021

Expose validators #109

Open

		@@ -0,0 +1,21 @@
		export const fileHeader = `/*
		* Copyright 2021-Present The Serverless Workflow Specification Authors

Comments / formatting #84

Comments / formatting #84

Uh oh!

Conversation

JBBianchi commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JBBianchi commented May 14, 2021

Uh oh!

JBBianchi commented May 15, 2021

Uh oh!

antmendoza commented May 16, 2021

Uh oh!

antmendoza commented May 16, 2021

Uh oh!

JBBianchi commented May 16, 2021

Uh oh!

JBBianchi commented May 17, 2021

Uh oh!

antmendoza 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!

JBBianchi commented May 17, 2021

Uh oh!

tsurdilo commented May 17, 2021

Uh oh!

JBBianchi commented May 18, 2021

Uh oh!

JBBianchi commented May 18, 2021

Uh oh!

Uh oh!

JBBianchi commented May 14, 2021 •

edited

Loading