Issue 7622: New properties template - Light - #7703
Conversation
324018f to
ddc1ed3
Compare
patchwork01
left a comment
There was a problem hiding this comment.
The outstanding comment on the storage property also needs resolving.
There was a problem hiding this comment.
I don't think this file is needed?
There was a problem hiding this comment.
Added for completeness sake and for future possible expansion.
Cost of inclusion is miminal so effort seemed warranted here.
There was a problem hiding this comment.
This shows every property, which isn't necessary. I think it seems like overkill for a "light" example. It also won't be valid, because there's no schema file.
If you want completeness, please match the basic example, including the tags and schema files.
There was a problem hiding this comment.
I think it's a bit misleading to have the light example header here, when there aren't properties set. Please match the basic example. Here's a link to the original thread: #7703 (comment)
|
|
||
| @Nested | ||
| @DisplayName("Generate basic table properties template") | ||
| class GenerateBasicTablePropertiesTemplate { |
There was a problem hiding this comment.
There's just one table properties template. Did you mean to revert this change?
I think the template tests should probably stay together, rather than have an example in between them.
There was a problem hiding this comment.
Will move next to one another
There was a problem hiding this comment.
The name and display name still need reverting.
There was a problem hiding this comment.
Now the name and display name have been reverted but the template tests are split apart again. Here's a link to the original thread: #7703 (comment)
patchwork01
left a comment
There was a problem hiding this comment.
This is all pretty minor now, though a couple of them are unresolved threads from before.
There was a problem hiding this comment.
I think it's a bit misleading to have the light example header here, when there aren't properties set. Please match the basic example. Here's a link to the original thread: #7703 (comment)
| public class GeneratePropertiesTemplates { | ||
|
|
||
| private static final String LIGHT_MODE_EXPLANATION = "# #\n" + | ||
| "# --- LIGHT MODE --- #\n" + |
There was a problem hiding this comment.
I think the idea of a "mode" is a bit misleading. It's just a collection of configuration properties, not a mode you can put an instance into. I'd just leave out this line, because the files are already under a "light" directory.
| writeFile(lightTemplateDir.resolve("instance.properties"), | ||
| GeneratePropertiesTemplates::writeExampleLightInstanceProperties); | ||
| writeFile(lightTemplateDir.resolve("table.properties"), | ||
| GeneratePropertiesTemplates::writeExampleLightTableProperties); |
There was a problem hiding this comment.
I think we can just use writeExampleBasicTableProperties here, rather than duplicating it?
| @@ -0,0 +1,108 @@ | |||
| ################################################################################# | |||
| # SLEEPER INSTANCE PROPERTIES # | |||
| ################################################################################# | |||
There was a problem hiding this comment.
I think this part of the header seems a bit unnecessary. We don't have a header like this in any of the other examples. I'd rather avoid matching how it's done in the templates, which we're going to delete.
|
|
||
| @Nested | ||
| @DisplayName("Generate basic table properties template") | ||
| class GenerateBasicTablePropertiesTemplate { |
There was a problem hiding this comment.
Now the name and display name have been reverted but the template tests are split apart again. Here's a link to the original thread: #7703 (comment)
Make sure you have checked all steps below.
Issue
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
Tests
Documentation
separate issue for that below.