Move generated schemas into sub-folders #1024
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roncohen
force-pushed
the
move-generated-schemas
branch
from
5fead4e to
cec4252
Compare
|
rebased and updated tests |
simitt
requested changes
Jun 27, 2018
| @@ -4,6 +4,7 @@ import ( | |||
| "testing" | |||
|
|
|||
| er "github.com/elastic/apm-server/processor/error" | |||
| generatedschemas "github.com/elastic/apm-server/processor/error/generated-schemas" | |||
There was a problem hiding this comment.
Pleas use schema over schemas - as Rakyll's post points out package names should not be plural but singular.
I am not a huge fan of having generated-schemas in the path. The package name is schemas, the import path ends with generated-schemas and it needs to be overwritten with generatedschemas - if not absolutely necessary I'd rather avoid that.
How about either
- getting rid of
generatedand just useschema - or use
generated/schemafolder structure to have a cleaner import. This would also allow to add further generated files in the future if necessary.
| {"sourcemaps/payload.json", "processor/sourcemap/schema.go", "sourcemapSchema", "sourcemap"}, | ||
| {"errors/payload.json", "processor/error/generated-schemas/payload.go", "PayloadSchema"}, | ||
| {"transactions/payload.json", "processor/transaction/generated-schemas/payload.go", "PayloadSchema"}, | ||
| {"sourcemaps/payload.json", "processor/sourcemap/generated-schemas/payload.go", "SourcemapSchema"}, |
There was a problem hiding this comment.
The name SourcemapSchema is not consistent - please also change to PayloadSchema.
roncohen
force-pushed
the
move-generated-schemas
branch
from
6ea8c8c to
bff5e83
Compare
|
thanks @simitt! Updated and rebased. |
|
jenkins, retest this please |
simitt
approved these changes
Jun 27, 2018
|
jenkins, retest this please |
simitt
pushed a commit
to simitt/apm-server
that referenced
this pull request
Jul 4, 2018
simitt
pushed a commit
to simitt/apm-server
that referenced
this pull request
Jul 4, 2018
simitt
pushed a commit
that referenced
this pull request
Jul 4, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This moves generated schemas into their own sub-folders in their respective processors.
This is extracted from the work on intake v2.
Right now, I think it's nice to have auto-generated and human-generated source code separately.
In the future for intake v2, we'll need to validate entities like errors and transactions separate from the payload, which means we'll need to have the schemas separate for the individual entities.
When we add the individual schemas (errors, transactions) in the future, we'd be putting even more auto-generated files in the same directories as the human-generated code, if we didn't have a sub-folder for it.
I went ahead and gave the schema constants names (
const PayloadSchema) instead of using theSchema()method, because we'll need to have more schemas in the package in the future.