search

SolomonDefi / solomon-monorepo

Monorepo containing core Solomon apps, services, libraries, and deploy config.
6 stars 3 forks source link

Common event json schema #155

Closed apolkingg8 closed 3 years ago

apolkingg8 commented 3 years ago

Close #146

apolkingg8 commented 3 years ago

Hi @kelvin-wong , I need your help to complete this PR:

  1. Please add a script to generate the JSON schema into libs/shared/util-event/schema, the file name should be xxx.schema.json.
  2. Add the script into CI, before the Generate event interface step.

Thanks a lot :)

solomondefi-dev commented 3 years ago

One thing I want to note is that all generated files should be committed to the repository, unless they are in the root /dist folder.

apolkingg8 commented 3 years ago

One thing I want to note is that all generated files should be committed to the repository, unless they are in the root /dist folder.

Why? As far as I know, the generated files should be ignored in git, and re-generated when the source change (just like what we do in the contract type). Isn't it?

solomondefi-dev commented 3 years ago

Hm, the contract types are already causing some trouble if the developer forgets to regenerate. I prefer for them to be included in the repo as well - it's nice to limit the number of steps between "git clone" and "run the project".

In general, generated files excluded from git should be considered build files, and should be generated to the root /dist folder. This is standard practice with Nx.

apolkingg8 commented 3 years ago

If we add the generated files into git, that will also cause some sync issue if the developer forgets to regenerate from the source. I think the better way is to add a git hook and do the following steps:

  1. check source changed or not
  2. check type is empty or not
  3. if changed or type is empty, regenerate files

Not surveyed it yet, but I think it's not so hard to implement. I don't think we should put every generated file into /dist, the project structure will be very weird if we do that.

apolkingg8 commented 3 years ago

Open #159 for the generator hook.

solomondefi-dev commented 3 years ago

Hey @kelvin-wong, can you try adding the script to CI, so that it passes? If it's easier, you could implement the hook discussed in #159 at the same time.

kelvin-wong commented 3 years ago

Hey @kelvin-wong, can you try adding the script to CI, so that it passes? If it's easier, you could implement the hook discussed in #159 at the same time.

OK. Let me look into it.

solomondefi-dev commented 3 years ago

@kelvin-wong @apolkingg8 Does it make sense to merge this now and cover CI problems in #159? I don't want this to get too stale and cause conflicts with other PRs.

kelvin-wong commented 3 years ago

The generation script is added to this pull request. I think my part is done here. My remaining work could continue on #159.

apolkingg8 commented 3 years ago

Ok, I'll let the CI pass here and merge it.

solomondefi-dev commented 3 years ago

Nice work guys, thanks!

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 1.3.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: