feat: Server SDK Diagrams + Generator Action #657
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @JamieSinn - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
.github/workflows/diagrams.yml
Outdated
| fetch-depth: 1 | ||
| - name: plantuml | ||
| id: plantuml | ||
| uses: kolchurinvv/generate-plantuml-action@master |
There was a problem hiding this comment.
suggestion (code_refinement): Consider pinning the action to a specific version instead of using master.
Using a specific version or commit hash helps in maintaining consistent behavior and avoiding unexpected changes when the master branch is updated.
| uses: kolchurinvv/generate-plantuml-action@master | |
| uses: kolchurinvv/generate-plantuml-action@<specific-version-or-commit-hash> |
JamieSinn
changed the title
| group Config Manager | ||
| group Constructor/Initialization | ||
| start | ||
| if (options.UseRealtimeUpdates) then (yes) |
There was a problem hiding this comment.
this option is named wrong here
There was a problem hiding this comment.
It's more "human" here - but if we want to make it exact that works too
There was a problem hiding this comment.
well either use words or use the name of the option if youre going to write it as options.Something
There was a problem hiding this comment.
I was trying to make it clear that the value is being pulled from the options passed into the initialization call - thoughts on how to better do that? Our options aren't named consistently
There was a problem hiding this comment.
they are named consistently in most cases. Any case where they aren't should be fixed
There was a problem hiding this comment.
The fact you said "most" means they aren't :P Plus - all server SDKs don't actually have this functionality enabled - so for the 1/1 for go - thoughts on using the options.EnableRealtimeUpdates ? or options.DisableRealtimeUpdates ? -
Go will be disabled by default - so will have Enabled as the option name
There was a problem hiding this comment.
it should be named the same as the existing option in the other SDKs
There was a problem hiding this comment.
and it should be enabled by default
There was a problem hiding this comment.
Until we're confident in it with the test harness - it's going to be disabled by default - cc @jonathannorris
The other SDKs this is relevant to (server sdks) do not have a current option - do we want to match the client sdks?
There was a problem hiding this comment.
yes, we want it to match the client SDKs
|
@JamieSinn and/or @ajwootto - where is this PR sitting right now? |
|
we'll probably make some logic changes in it as the beta works through in go/nodejs - so pending for now. |
I don't know whether or not the actual SVG's should be rendered in the features location - but there's no other "Server SDK" global docs.