Add nitpicking CI integration #10937
Conversation
kkaefer
force-pushed
the
verify
branch
4 times, most recently
from
851b7ae
to
c709ca0
Compare
|
If you forget to run code generators, or have an incorrect submodule pin, the A failing build looks like this:
|
| @@ -3364,6 +3364,7 @@ | |||
| return new LayoutPropertyValue<>("text-optional", value); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
What introduced this extra newline?
There was a problem hiding this comment.
Unsure, but that's what the code generator produces. Might be some change in the template, or someone forgot to commit this newline from the generator, e.g. in 757cc0f
There was a problem hiding this comment.
This looks great. I didn't get too far into looking at whether the code was correct, but it looks about right!
Would it make sense to include instructions on temporary modification (e.g. in style-spec.js and generate-shaders.js, as well as the file lists) in the failure message? I imagine over time a lot of people's first introduction to these scripts will be via an unexpected failure on CI.
| @@ -0,0 +1,36 @@ | |||
| platform/android/src/style/light.hpp | |||
There was a problem hiding this comment.
Could we move all generated files to /generated folders or rename them <filename>.gen.<ext>.
It would remove the need for the generate-style-code-list files, and (in addition to the comments at the top of the file) make it obvious when generated files are being viewed/modified.
There was a problem hiding this comment.
For iOS and macOS, it wouldn’t be practical to rename the files, since the header file names are exposed as-is to developers and the Markdown file names show up in URLs in the API reference. However, moving generated files to several /generated folders would be straightforward, since Xcode’s project structure is an abstraction of the file system anyways. Note that the generated guides to have to live more or less where they do now.
There was a problem hiding this comment.
The iOS contributing guide and its macOS counterpart probably need a new section about generating runtime styling code, since the new .list files aren’t particularly discoverable.
| @@ -0,0 +1,6 @@ | |||
| var spec = module.exports = require('../mapbox-gl-js/src/style-spec/reference/v8'); | |||
|
|
|||
| // Make temporary modifications here when Native doesn't have all features that JS has. | |||
There was a problem hiding this comment.
Centralizing these modifications is a great idea. 👍
|
@asheemmamoowala @1ec5 The .list files are automatically generated too from the code gen scripts and don't have to managed by the user. Instead of checking them in, we can also add them to .gitignore, since they'll just serve as a way for the nitpick script to know what files are auto-generated, and the nitpick script runs the code generator beforehand anyway. |
This ensures that the generated code matches what is checked in. Remove those temporary modifications once we add the features to master.
Bash sorting is weird...
…ipts This will help us tracking deleted/modified/added files
The idea of this build is to be optional and to fail when these requirements haven't been met as a reminder to run this code.