RBS improvements #174
RBS improvements #174
Conversation
codegen/projections/high_score_service/sig/high_score_service/client.rbs
Outdated
Show resolved
Hide resolved
This reverts commit 74fc422.
This reverts commit ec5f7ab.
codegen/projections/high_score_service/sig/high_score_service/client.rbs
Outdated
Show resolved
Hide resolved
codegen/projections/high_score_service/sig/high_score_service/client.rbs
Outdated
Show resolved
Hide resolved
| CI_HEARTH_PATH = "../hearth/sig" | ||
| PROJECTIONS_HEARTH_PATH = "../../../hearth/sig" | ||
| HEART_SIG_DIR = Dir.exist?(CI_HEARTH_PATH) ? CI_HEARTH_PATH : PROJECTIONS_HEARTH_PATH |
There was a problem hiding this comment.
What about using ENV variables from CI instead? Or "working directory" in CI and use the current dir (preferred)?
| @@ -0,0 +1,30 @@ | |||
| require 'rubygems' | |||
There was a problem hiding this comment.
Should this Rakefile actually live in the main repo root and be copied into projections if needed? Similar to how v3 repo is setup.
There was a problem hiding this comment.
I'm not sure - its specific to whitelabel (in namespace)
| @@ -1,5 +1,6 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| # Used to test endpoint and other models from Smithy | |||
There was a problem hiding this comment.
Possibly RBS test definition can be here? IDK
There was a problem hiding this comment.
I dont think so - this works on a different context. We could jam it together, we just would then need to make sure to never run these tests from the whitelabel. And vice-versa.
There was a problem hiding this comment.
RE: All of the rakefile comments - I'll take a broader look at what we can do here in a new PR. I think we need to throw all of our existing CI away and redo it if we want a more unified, single Rakefile
| @@ -0,0 +1,30 @@ | |||
| require 'rubygems' | |||
There was a problem hiding this comment.
Ditto on Rakefile comment - shouldn't we just copy it like we do Steepfile etc? This seems duplicative.
There was a problem hiding this comment.
We do just copy this rakefile - I added a new task in the build that copies it (right after the steepfile gets copied).
| .definitionFile(DEFAULT_DEFINITION_FILE).build(); | ||
| } | ||
|
|
||
| @Override | ||
| public Symbol unionShape(UnionShape shape) { | ||
| String name = getDefaultShapeName(shape, "Union__"); | ||
| return createSymbolBuilder(shape, name, name, name, moduleName) | ||
| return createSymbolBuilder(shape, name, "Types::" + name, name, moduleName) |
There was a problem hiding this comment.
Is this correct? Some times we may not want to print out a full qualified name. I would assume it's relative to where it's being written.
There was a problem hiding this comment.
Yeah - I was debating this. The rbsType property doesn't have the same namespace sensitive ability as the actual symbol name. I think Types::<name> will always be correct (it just may be more information than necessary if you use it within the types module).
Edit: I've confirmed that these types in the generated types.rbs still work correctly (and pass validation).
Fixes #172