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

feat(typescript): add rcbc #12

Merged
merged 5 commits into from
Apr 25, 2023
Merged

feat(typescript): add rcbc #12

merged 5 commits into from
Apr 25, 2023

Conversation

bearmit
Copy link
Contributor

@bearmit bearmit commented Apr 21, 2023

This pull request is related to the card: https://trello.com/c/BQvuiXmu/39-cript-scripts-rcbc

What does this script?

The script just convert static JS objects to JSON.

The original power point graphs and the generated JSON are available here: https://drive.google.com/drive/folders/1UXFJX_NNChygw-vJOwtZY6GI90qdKGvx

General improvements (shared with all scripts)

  • The CriptJSON static class generate references ("uid": "_:NAME") only when necessary
  • We use streams instead of regular file write

@bearmit bearmit self-assigned this Apr 21, 2023
@nh916
Copy link
Contributor

nh916 commented Apr 21, 2023

I don't know enough to provide an effective review, but I ran this PR locally and it seems to work fine.

The only note I have is regarding Link to TypeScript source code and I think that could be improved and changed to instead say TypeScript source code on GitHub. I think that could also earn us some SEO points as well.
I wrote a small SEO links wiki within the CRIPT Scripts repository that I thought could be helpful for us

I leave it for you to merge this PR in whenever you think it is good, and then please create a release, generate release notes, and let the CRIPT channel know. Or just let me know when you merge it in and I can do all of that!
These steps should eventually be automated if we decide to stick with this repository for the long term

@bearmit
Copy link
Contributor Author

bearmit commented Apr 21, 2023

@nh916 Thanks. I would like to wait a little bit in case @shijiale0609 have a moment to look at the graph sources (in src/rcbc/data/graph) next week.

Copy link

@shijiale0609 shijiale0609 left a comment

Choose a reason for hiding this comment

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

computations.ts, PPV-b-PI-89. There is no computation in the graph which was polished by Brad. see the last slide https://docs.google.com/presentation/d/1eXM7870YM2sxQQvMFahI2Ox8-EFCGaw_/edit#slide=id.p3

@bearmit bearmit merged commit d72f979 into master Apr 25, 2023
@nh916
Copy link
Contributor

nh916 commented Apr 25, 2023

@bearmit be sure to delete branches that you are not using, its just good to keep the repository clean

@bearmit bearmit deleted the feat/rcbc branch April 25, 2023 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants