Closes #1289 - Create a component for generating configuration documentations #1293
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1293 +/- ##
============================================
- Coverage 83.00% 80.80% -2.20%
- Complexity 1736 2026 +290
============================================
Files 174 204 +30
Lines 5366 6460 +1094
Branches 650 769 +119
============================================
+ Hits 4454 5220 +766
- Misses 675 950 +275
- Partials 237 290 +53
|
There was a problem hiding this comment.
Reviewed 21 of 73 files at r1.
Reviewable status: 15 of 74 files reviewed, 27 unresolved discussions (waiting on @aaronweissler and @MariusBrill)
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigDocManager.java, line 7 at r1 (raw file):
import rocks.inspectit.ocelot.config.model.InspectitConfig; public class ConfigDocManager {
I am not quite sure what this class is needed for - let's talk about it.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 35 at r1 (raw file):
//Parse the InspectitConfig from the created YAML String ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
It should be sufficient to instantiate the objectMapper only once in the constructor of the ConfigParser instead of creating a new one with each method call
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 44 at r1 (raw file):
//In the YAML property-names are kebab-case in the Java objects CamelCase, Jackson can do that conversion //with the following line mapper.setPropertyNamingStrategy(PropertyNamingStrategies.KEBAB_CASE);
Same as with ObjectMapper - just instantiate once in the constructor of this class
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 62 at r1 (raw file):
* @return The YAML-String with the placeholders replaced. */ protected String replacePlaceholders(String configYaml){
Is there a reason this method is protected instead of private?
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 65 at r1 (raw file):
//deserialize YAML to Map to get the placeholders' values from ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
See ObjectMapper comment above
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 71 at r1 (raw file):
//find the first occurence of the ${placeholder} syntax int index = configYaml.indexOf("${"); while (index >= 0) {
I think you could make this whole method easier by using the StringSubstitutor: https://commons.apache.org/proper/commons-text/javadocs/api-release/org/apache/commons/text/StringSubstitutor.html
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 90 at r1 (raw file):
index = configYaml.indexOf("${"); } } catch (IOException e) {
If you just print out StackTraces you can also just add a throws-declaration to the method instead of catching the Exception and printing it.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 103 at r1 (raw file):
* @return The value from within the nested Maps. */ private String getNestedValue(Map<String, Object> map, List<String> keys) {
Should be obsolete if you use the StringSubstitutor. If not we should have a chat on what excactly this method does.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 139 at r1 (raw file):
* @return Returns the corresponding Duration object. */ private static Duration parseHuman(String text) {
Lets talk about this method. I think it can be simplified using something like SimpleDateFormat
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ActionDoc.java, line 10 at r1 (raw file):
public class ActionDoc extends BaseDoc { public ActionDoc(String name, String description, List<ActionInputDoc> inputs, String returnDescription, Boolean isVoid) {
Lombok should automatically generate constructors with the @data-annotation so no need to write them yourself.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ActionDoc.java, line 17 at r1 (raw file):
} List<ActionInputDoc> inputs;
Is there a reason these variables are package-private?
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ActionInputDoc.java, line 8 at r1 (raw file):
public class ActionInputDoc{ public ActionInputDoc(String name, String type, String description){
Lombok should automatically generate constructors with the @data-annotation so no need to write them yourself.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ActionInputDoc.java, line 14 at r1 (raw file):
} String name;
See ActionDoc
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/BaseDoc.java, line 13 at r1 (raw file):
} String description;
See ActionDoc
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ConfigDocumentation.java, line 10 at r1 (raw file):
public class ConfigDocumentation { public ConfigDocumentation(List<ScopeDoc> scopes, List<ActionDoc> actions, List<RuleDoc> rules, List<MetricDoc> metrics) {
Lombok should automatically generate constructors with the @data-annotation so no need to write them yourself.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/DocObjectGenerator.java, line 36 at r1 (raw file):
Map<String, InstrumentationScopeSettings> scopes = instrumentation.getScopes(); Map<String, GenericActionSettings> actions = instrumentation.getActions(); Map<String, InstrumentationRuleSettings> rules = instrumentation.getRules();
Let's talk about the structure of this method - I think we can refactore it to increase readability
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/DocObjectGenerator.java, line 58 at r1 (raw file):
} private List<ScopeDoc> generateScopeDocs(Map<String, InstrumentationScopeSettings> scopes){
Let's talk about these generate-methods. I think there could be a way to generify them.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/MetricDoc.java, line 6 at r1 (raw file):
@Getter public class MetricDoc extends BaseDoc {
I think you can also use the Data-Annotation here
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/MetricDoc.java, line 13 at r1 (raw file):
} String unit;
Can also be private.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDoc.java, line 27 at r1 (raw file):
} List<String> include = Collections.emptyList();
These variables should also be private
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDoc.java, line 35 at r1 (raw file):
public void addEntryExitFromIncludedRules(Map<String, RuleDoc> allRuleDocs, List<String> includedRules){ for(String includedRuleName: includedRules){
I think you could increase the readability of this method by using streams.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleMetricsDoc.java, line 7 at r1 (raw file):
import java.util.Map; @Getter
If you don't want to use the Data-Annoation, you can also use AllArgsConstructor to auto-generate a constructor for your class
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleMetricsDoc.java, line 17 at r1 (raw file):
} String name;
Can be private
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleTracingDoc.java, line 7 at r1 (raw file):
import java.util.Map; @Getter
Again - just generate the constructor with an annotation
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleTracingDoc.java, line 16 at r1 (raw file):
} Boolean startSpan;
Can be private
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigDocManagerTest.java, line 29 at r1 (raw file):
@BeforeAll static void createDocObjects(){
It's hard to comprehend this method without any docs - maybe describe in a few sentences what you do
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigDocManagerTest.java, line 107 at r1 (raw file):
} ConfigDocManager configDocManager = new ConfigDocManager();
Let's talk about testing in general - you can make things easier by using Mockito
aaronweissler
force-pushed
the
feature/config-doc-generator
branch
from
4cac2ee to
13b8f0f
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 77 files reviewed, 27 unresolved discussions (waiting on @aaronweissler and @MariusBrill)
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigDocManager.java, line 7 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I am not quite sure what this class is needed for - let's talk about it.
Done.
Discussion result: Keep for now but talk about it again with next related PR.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ActionDoc.java, line 10 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Lombok should automatically generate constructors with the @data-annotation so no need to write them yourself.
Unfortunately, Lombok's constructor generation apparently does not play nice with inheritance, because it would only call the super-class's base constructor without any arguments and also only generate a constructor for the child-class with its own fields and not include the super-class's fields.
See for example: https://stackoverflow.com/questions/29740078/how-to-call-super-constructor-in-lombok/29771875
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ActionDoc.java, line 17 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Is there a reason these variables are package-private?
No, I think I just forgot to change the default.
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ActionInputDoc.java, line 8 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Lombok should automatically generate constructors with the @data-annotation so no need to write them yourself.
Done.
See comment regarding Lombok and inheritance above.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ActionInputDoc.java, line 14 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
See ActionDoc
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/BaseDoc.java, line 13 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
See ActionDoc
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ConfigDocumentation.java, line 10 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Lombok should automatically generate constructors with the @data-annotation so no need to write them yourself.
Done.
Here I actually can use it, since it doesn't involve inheritance.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/DocObjectGenerator.java, line 58 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Let's talk about these generate-methods. I think there could be a way to generify them.
I can't really find a way to generify them nicely, because I think I'd need a superclass for all of them that has the method getDoc(), but introducing inheritance for them just for saving the about seven lines that are the same in three of the methods, doesn't seem worth it (also considering how that might behave with Lombok as discovered earlier).
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/MetricDoc.java, line 6 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I think you can also use the Data-Annotation here
Done.
See comment regarding Lombok and inheritance above.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/MetricDoc.java, line 13 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Can also be private.
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDoc.java, line 27 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
These variables should also be private
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDoc.java, line 35 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I think you could increase the readability of this method by using streams.
I have committed a version with streams, I'm not entirely sure if it's much more readable, let me know what you think of it.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleMetricsDoc.java, line 7 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
If you don't want to use the Data-Annoation, you can also use AllArgsConstructor to auto-generate a constructor for your class
Done.
Here I actually can use it, since it doesn't involve inheritance.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleMetricsDoc.java, line 17 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Can be private
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleTracingDoc.java, line 7 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Again - just generate the constructor with an annotation
Done.
Here I actually can use it, since it doesn't involve inheritance.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleTracingDoc.java, line 16 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Can be private
Done.
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigDocManagerTest.java, line 29 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
It's hard to comprehend this method without any docs - maybe describe in a few sentences what you do
Done.
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigDocManagerTest.java, line 107 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Let's talk about testing in general - you can make things easier by using Mockito
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 35 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
It should be sufficient to instantiate the objectMapper only once in the constructor of the ConfigParser instead of creating a new one with each method call
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 44 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Same as with ObjectMapper - just instantiate once in the constructor of this class
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 62 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Is there a reason this method is protected instead of private?
Done.
Should have had VisibleForTesting annotation, is however not in the new version with StringSubstitutor anyways.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 65 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
See ObjectMapper comment above
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 71 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I think you could make this whole method easier by using the StringSubstitutor: https://commons.apache.org/proper/commons-text/javadocs/api-release/org/apache/commons/text/StringSubstitutor.html
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 90 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
If you just print out StackTraces you can also just add a throws-declaration to the method instead of catching the Exception and printing it.
Done.
This part of the code was moved elsewhere where the Exception is not only caught to print a stacktrace due to the refactoring to use StringSubstitutor. However I am thinking about throwing the JsonProcessingException there too, could maybe also discuss that again with the next PR, since the contents of that are the reason for my considerations.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 103 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Should be obsolete if you use the StringSubstitutor. If not we should have a chat on what excactly this method does.
Done.
Unfortunately, StringSubstitutor only expects a flat Map as its valueMap, so this method is still needed and employed in the new custom StringLookupNestedMap class.
Because of that, we could still talk about what exactly it does, if you want to.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigParser.java, line 139 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Lets talk about this method. I think it can be simplified using something like SimpleDateFormat
Done.
As talked about, as far as I understand, the method is needed to deal with the fact that Jackson wants to use Duration.parse to parse a String into a Duration, but parse expects a String in the ISO-8601 duration format (e.g. PT20.5S) and the values in our YAMLs are in a different more human-readable format.
There was a problem hiding this comment.
Reviewed 45 of 54 files at r2, 13 of 29 files at r4.
Reviewable status: 59 of 77 files reviewed, 17 unresolved discussions (waiting on @aaronweissler and @MariusBrill)
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ScopeDoc.java, line 3 at r4 (raw file):
package inspectit.ocelot.config.doc.generator.docobjects; public class ScopeDoc extends BaseDoc {
I guess you added this white space by mistake?
components/inspectit-ocelot-config-doc-generator/src/main/java/parsing/ConfigParser.java, line 19 at r4 (raw file):
@Slf4j public class ConfigParser {
Also add a small documentation on what this class is used for
components/inspectit-ocelot-config-doc-generator/src/main/java/parsing/ConfigParser.java, line 34 at r4 (raw file):
} ObjectMapper mapper;
Can be private
components/inspectit-ocelot-config-doc-generator/src/main/java/parsing/ConfigParser.java, line 66 at r4 (raw file):
* @return Returns the corresponding Duration object. */ private static Duration parseHuman(String text) {
I think this method belongs to CustomDurationDeserializer
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigDocManagerTest.java, line 109 at r4 (raw file):
} ConfigDocManager configDocManager = new ConfigDocManager();
Just use @Injectmocks
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigDocManagerTest.java, line 166 at r4 (raw file):
ConfigDocumentation expected = new ConfigDocumentation(Collections.emptyList(), actions, Collections.emptyList(), Collections.emptyList()); assertEquals(expected, result);
Instead of using one assertEquals statement create one for each variable in ConfigDocumentation - this way we can find errors easier as you can see which assertion fails.
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigParserTest.java, line 31 at r4 (raw file):
class ConfigParserTest { private final ConfigParser configParser = new ConfigParser();
Use @Injectmocks
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigParserTest.java, line 58 at r4 (raw file):
variable1, variable2, variable3, variable4); ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
Instead of using an instance of ObjectMapper you can just mock the class as we don't want to test it in the unit test.
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigParserTest.java, line 107 at r4 (raw file):
pollingSettings.setEnabled(enabledMap); MetricDefinitionSettings metricDefinition = MetricDefinitionSettings.builder()
Just use mocks instead of instances of the Settings-Objects
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigParserTest.java, line 193 at r4 (raw file):
actions.put("a_debug_println", action); InstrumentationSettings instrumentation = new InstrumentationSettings();
Again - just use mocks
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDocTest.java, line 124 at r4 (raw file):
RuleActionCallDoc overwritten_by_2 = expected.get(RULE_2_NAME).getEntryExits().get(ENTRY_KEY).get(OVERWRITTEN_BY_2); RuleActionCallDoc inherited_from_2 = new RuleActionCallDoc(overwritten_by_2, RULE_2_NAME); expected.get(RULE_1_NAME).getEntryExits().get(ENTRY_KEY).put(OVERWRITTEN_BY_2, inherited_from_2);
Just use mocks
inspectit-ocelot-config/src/main/resources/rocks/inspectit/ocelot/config/default/instrumentation/actions/_shared/regex.yml, line 230 at r4 (raw file):
} return null;
Ist there a reason you removed this config?
(However a custom StringLookup class was needed, because StringSubstitutor normally would not support nested Maps as the valueMap)
Make fields private and final where possible. Let Lombok generate Constructors where possible.
Move YAML from yml-files into Strings to improve understandability by having everything in one place. Fix error in RuleDocTest (no assertion was made). Use Nested test classes.
Add some documentation Move method Set field as private
aaronweissler
force-pushed
the
feature/config-doc-generator
branch
from
9c93058 to
f7b85a5
Compare
There was a problem hiding this comment.
Reviewable status: 50 of 77 files reviewed, 17 unresolved discussions (waiting on @MariusBrill)
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/DocObjectGenerator.java, line 36 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Let's talk about the structure of this method - I think we can refactore it to increase readability
Done.
See comment below
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/DocObjectGenerator.java, line 58 at r1 (raw file):
Previously, aaronweissler wrote…
I can't really find a way to generify them nicely, because I think I'd need a superclass for all of them that has the method getDoc(), but introducing inheritance for them just for saving the about seven lines that are the same in three of the methods, doesn't seem worth it (also considering how that might behave with Lombok as discovered earlier).
Done.
Talked about it, not possible with the current architecture.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ScopeDoc.java, line 3 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I guess you added this white space by mistake?
Didn't the most recent change remove it, or did I add it accidentally when force pushing the oldercommit with some changes?
But yes, it probably came from some annotation that I removed before and forgot to remove the full line as well.
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDocTest.java, line 124 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Just use mocks
Done
inspectit-ocelot-config/src/main/resources/rocks/inspectit/ocelot/config/default/instrumentation/actions/_shared/regex.yml, line 230 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Ist there a reason you removed this config?
Done
I mean, yes, I removed it on purpose while debugging, but no, of course it was not supposed to stay in the commit ^^
components/inspectit-ocelot-config-doc-generator/src/main/java/parsing/ConfigParser.java, line 19 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Also add a small documentation on what this class is used for
Done
components/inspectit-ocelot-config-doc-generator/src/main/java/parsing/ConfigParser.java, line 34 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Can be private
Done
components/inspectit-ocelot-config-doc-generator/src/main/java/parsing/ConfigParser.java, line 66 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I think this method belongs to CustomDurationDeserializer
Done
Yeah, makes more sense to just put it in there.
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigDocManagerTest.java, line 109 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Just use @Injectmocks
This file was refactored, so this specific line isn't in it anymore anyways, but for the analogous line, the same is the case as within ConfigParserTest, it wouldn't create any object, because there are no mocks to inject.
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigDocManagerTest.java, line 166 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Instead of using one assertEquals statement create one for each variable in ConfigDocumentation - this way we can find errors easier as you can see which assertion fails.
Done.
Actually using assertj's usingRecursiveComparison instead now.
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigParserTest.java, line 31 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Use @Injectmocks
I don't think it's possible, because, since I don't have any mocks to add to it, it wouldn't create any ConfigParser object at all leading to a NullPointerException
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigParserTest.java, line 58 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Instead of using an instance of ObjectMapper you can just mock the class as we don't want to test it in the unit test.
As talked about, here it is used and therefore needed to make creating the Map from the YAML String possible
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigParserTest.java, line 107 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Just use mocks instead of instances of the Settings-Objects
Done
components/inspectit-ocelot-config-doc-generator/src/test/java/inspectit/ocelot/config/doc/generator/ConfigParserTest.java, line 193 at r4 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Again - just use mocks
Done
For one I think it represents the reality better, the descriptions aren't intentionally empty, they simply do not exist.
There was a problem hiding this comment.
Reviewed 1 of 73 files at r1, 4 of 54 files at r2, 5 of 29 files at r4, 9 of 14 files at r6, 2 of 2 files at r7, 1 of 2 files at r8.
Reviewable status: 59 of 77 files reviewed, 39 unresolved discussions (waiting on @aaronweissler and @MariusBrill)
settings.gradle, line 13 at r9 (raw file):
project(':inspectit-ocelot-configurationserver').projectDir = new File('./components/inspectit-ocelot-configurationserver') project(':inspectit-ocelot-eum-server').projectDir = new File('./components/inspectit-ocelot-eum-server') project(':inspectit-ocelot-config-doc-generator').projectDir = new File('./components/inspectit-ocelot-config-doc-generator')
How about naming this project: inspecit-ocelot-configdocsgenerator or inspecit-ocelot-docsgenerator?
Code quote:
inspectit-ocelot-config-doc-generatorcomponents/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigDocManager.java, line 1 at r9 (raw file):
package inspectit.ocelot.config.doc.generator;
Please use the package name inspectit.ocelot.docsgenerator
Code quote:
inspectit.ocelot.config.doc.generatorcomponents/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigDocManager.java, line 1 at r9 (raw file):
package inspectit.ocelot.config.doc.generator;
Please use in the naming (and in the model docsinstead of doc). Imo, docs is more likely to be associated with documentation than doc.
Code quote:
doccomponents/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigDocManager.java, line 8 at r9 (raw file):
import rocks.inspectit.ocelot.config.model.InspectitConfig; public class ConfigDocManager {
It's more like a Generator instead of a Manager
Code quote:
ConfigDocManagercomponents/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ActionDoc.java, line 18 at r9 (raw file):
} private final List<ActionInputDoc> inputs;
Fields should be located above the constructor.
Please use this ordering in all classes.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/BaseDoc.java, line 8 at r9 (raw file):
@RequiredArgsConstructor @Getter public abstract class BaseDoc {
Please move all "data" classes into a separate package, e.g.: ..generator.model
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/BaseDoc.java, line 10 at r9 (raw file):
public abstract class BaseDoc { private final String name;
Please format all files with the ocelot code style: https://github.com/inspectIT/inspectit-ocelot/blob/master/codequality/idea/code_style.xml
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/DocObjectGenerator.java, line 27 at r9 (raw file):
public class DocObjectGenerator { public ConfigDocumentation generateConfigDocumentation(InspectitConfig config){
Please add documentation to the public methods and classes (what are they doing and what are the arguments for).
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleActionCallDoc.java, line 19 at r9 (raw file):
} private final String name;
ruleName
Code quote:
namecomponents/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleActionCallDoc.java, line 20 at r9 (raw file):
private final String name; private final String action;
actionName
Code quote:
actioncomponents/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleActionCallDoc.java, line 21 at r9 (raw file):
private final String name; private final String action; private String inheritedFrom;
What is this? Is it a name of a rule?
Code quote:
inheritedFromcomponents/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDoc.java, line 14 at r9 (raw file):
public class RuleDoc extends BaseDoc { public static String[] possibleValuesEntryExits = {"preEntry", "entry", "postEntry", "preExit", "exit", "postExit"};
Please use an enum for this. It's easier to use for iteration and makes it less error prone.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDoc.java, line 30 at r9 (raw file):
As of now only needed for simpler code in RuleDocTest. */ public RuleDoc(String name){
Please remove this if it is only imlemented for testing. Implementing code which is only there because of testing is imo not a good option.
In case you want to simplify tests you could use helper methods in the tests itself or something like that.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDoc.java, line 34 at r9 (raw file):
} private List<String> include = Collections.emptyList();
Fields above cosntructor.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/actions/GenericActionSettings.java, line 55 at r9 (raw file):
/** * Documentation for Config-Docs generation
Javadocs should end with a dot .
Code quote:
Documentation for Config-Docs generationinspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/actions/GenericActionSettings.java, line 57 at r9 (raw file):
* Documentation for Config-Docs generation */ private ActionDocSettings doc;
Please use docs. Same for scopes and rules.
Code quote:
docinspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/documentation/ActionDocSettings.java, line 14 at r9 (raw file):
public class ActionDocSettings extends BaseDocSettings { private Map<@NotBlank String, @NotBlank String> inputDesc = new HashMap<>();
I would prefer not to use the suffix Desc. Maybe just: inputs
Code quote:
inputDescinspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/documentation/ActionDocSettings.java, line 15 at r9 (raw file):
private Map<@NotBlank String, @NotBlank String> inputDesc = new HashMap<>(); private String returnDesc;
returnValue would be more expressive imo
Code quote:
returnDescinspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/documentation/BaseDocSettings.java, line 6 at r9 (raw file):
@Data public class BaseDocSettings {
Please add some documentation to the class.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/documentation/BaseDocSettings.java, line 6 at r9 (raw file):
@Data public class BaseDocSettings {
Since this class is not a container for "settings", I would propose to name it in something like: BaseDocumentation. Then the one used in actions can be named ActionDocumentation
Code quote:
BaseDocSettingscomponents/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/parsing/ConfigParser.java, line 37 at r9 (raw file):
} private final ObjectMapper mapper;
Fields above constructor.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/parsing/StringSubstitutorNestedMap.java, line 10 at r9 (raw file):
* Version of StringSubstitutor with the possibility to use a nested Map to define values for the replaced variables. */ public class StringSubstitutorNestedMap extends StringSubstitutor {
You can merge both of these classes into a single one:
public class StringSubstitutorNestedMap extends StringSubstitutor implements StringLookup {
private final Map<String, Object> valueMap;
public StringSubstitutorNestedMap(Map<String, Object> valueMap){
this.valueMap = valueMap;
this.setVariableResolver(this);
}
@Override
public String lookup(String variableName){
List<String> keys = Arrays.asList(variableName.split("\\."));
Object value = valueMap;
StringBuilder old_key = new StringBuilder();
for (String key : keys) {
//needs to be casted each time because Java does not know that within the Map there are more Maps
Object new_value = ((Map) value).get(old_key + key);
//Some keys themselves contain dots again, which previously were used as split points, e.g. there is a key
//concurrent.phase.time which points to one boolean value, so if no value is found with one key, it is
// concatenated with the next one on the next round of the loop.
if (new_value != null) {
//If that is not the case, simply replace the old Map with the newly found one.
value = new_value;
} else {
if(keys.get(keys.size() - 1).equals(key)){
// if the corresponding value can not be found, return null which leads to the full variable being
// kept in the YAML
return null;
} else {
old_key.append(key).append(".");
}
}
}
return value.toString();
}
}
Renaming project, classes and fields; changing package structure; add more javadocs.
There was a problem hiding this comment.
Reviewable status: 1 of 89 files reviewed, 39 unresolved discussions (waiting on @MariusBrill and @mariusoe)
settings.gradle, line 13 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
How about naming this project:
inspecit-ocelot-configdocsgeneratororinspecit-ocelot-docsgenerator?
Done.
Renamed to inspectit-ocelot-configdocsgenerator
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/actions/GenericActionSettings.java, line 55 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Javadocs should end with a dot
.
Done.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/actions/GenericActionSettings.java, line 57 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please use
docs. Same for scopes and rules.
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigDocManager.java, line 1 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please use the package name
inspectit.ocelot.docsgenerator
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigDocManager.java, line 1 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please use in the naming (and in the model
docsinstead ofdoc). Imo,docsis more likely to be associated with documentation thandoc.
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/ConfigDocManager.java, line 8 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
It's more like a
Generatorinstead of aManager
Done.
Actually also refactored this to combine this class and the DocObjectGenerator.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/ActionDoc.java, line 18 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Fields should be located above the constructor.
Please use this ordering in all classes.
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/BaseDoc.java, line 8 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please move all "data" classes into a separate package, e.g.:
..generator.model
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/BaseDoc.java, line 10 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please format all files with the ocelot code style: https://github.com/inspectIT/inspectit-ocelot/blob/master/codequality/idea/code_style.xml
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/DocObjectGenerator.java, line 27 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please add documentation to the public methods and classes (what are they doing and what are the arguments for).
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleActionCallDoc.java, line 19 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
ruleName
Done.
Discussed it, name is correct. Added JavaDoc to make it more understandable.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleActionCallDoc.java, line 20 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
actionName
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleActionCallDoc.java, line 21 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
What is this? Is it a name of a rule?
Done.
Added JavaDoc to explain it.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDoc.java, line 14 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please use an enum for this. It's easier to use for iteration and makes it less error prone.
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDoc.java, line 30 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please remove this if it is only imlemented for testing. Implementing code which is only there because of testing is imo not a good option.
In case you want to simplify tests you could use helper methods in the tests itself or something like that.
Done.
Is now a helper method in Test class.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/docobjects/RuleDoc.java, line 34 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Fields above cosntructor.
Done.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/documentation/ActionDocSettings.java, line 14 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
I would prefer not to use the suffix
Desc. Maybe just:inputs
Done.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/documentation/ActionDocSettings.java, line 15 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
returnValuewould be more expressive imo
Done.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/documentation/BaseDocSettings.java, line 6 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Please add some documentation to the class.
Done.
inspectit-ocelot-config/src/main/java/rocks/inspectit/ocelot/config/model/instrumentation/documentation/BaseDocSettings.java, line 6 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Since this class is not a container for "settings", I would propose to name it in something like:
BaseDocumentation. Then the one used in actions can be namedActionDocumentation
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/parsing/ConfigParser.java, line 37 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Fields above constructor.
Done.
components/inspectit-ocelot-config-doc-generator/src/main/java/inspectit/ocelot/config/doc/generator/parsing/StringSubstitutorNestedMap.java, line 10 at r9 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
You can merge both of these classes into a single one:
public class StringSubstitutorNestedMap extends StringSubstitutor implements StringLookup { private final Map<String, Object> valueMap; public StringSubstitutorNestedMap(Map<String, Object> valueMap){ this.valueMap = valueMap; this.setVariableResolver(this); } @Override public String lookup(String variableName){ List<String> keys = Arrays.asList(variableName.split("\\.")); Object value = valueMap; StringBuilder old_key = new StringBuilder(); for (String key : keys) { //needs to be casted each time because Java does not know that within the Map there are more Maps Object new_value = ((Map) value).get(old_key + key); //Some keys themselves contain dots again, which previously were used as split points, e.g. there is a key //concurrent.phase.time which points to one boolean value, so if no value is found with one key, it is // concatenated with the next one on the next round of the loop. if (new_value != null) { //If that is not the case, simply replace the old Map with the newly found one. value = new_value; } else { if(keys.get(keys.size() - 1).equals(key)){ // if the corresponding value can not be found, return null which leads to the full variable being // kept in the YAML return null; } else { old_key.append(key).append("."); } } } return value.toString(); } }
Done.
change 'doc' to 'docs'
There was a problem hiding this comment.
Reviewed 18 of 76 files at r10, all commit messages.
Reviewable status: 18 of 89 files reviewed, 41 unresolved discussions (waiting on @aaronweissler, @MariusBrill, and @mariusoe)
components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/ConfigDocsGenerator.java, line 30 at r11 (raw file):
*/ public class ConfigDocsGenerator {
As far as I see, there is no reason why you should create multiple instances of this class. You just want to use the generate.. method.
In order to rpevent unneccessary object creating, we can use a singleton pattern here.
Just create one instance, that will be used everywhere.
public static final ConfigDocsGenerator INSTANCE = new ConfigDocsGenerator();
private ConfigDocsGenerator() {
}
or
private static final ConfigDocsGenerator INSTANCE = new ConfigDocsGenerator();
public static ConfigDocsGenerator getInstance() {
return INSTANCE;
}
private ConfigDocsGenerator() {
}
components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/model/MetricDocs.java, line 18 at r11 (raw file):
public MetricDocs(String name, String description, String unit) {
Remove the empty line please :)
components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/model/MetricDocs.java, line 20 at r11 (raw file):
// MetricDefinitions do not contain the info for the since field, so it is left empty super(name, description, "");
Imo null is better to represent that it does not exists.
Code quote:
""components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/model/RuleDocs.java, line 99 at r11 (raw file):
@Getter @RequiredArgsConstructor public enum EntryExitKey {
You can move this into a own file because it is used in multiple classes.
components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/model/RuleDocs.java, line 99 at r11 (raw file):
@Getter @RequiredArgsConstructor public enum EntryExitKey {
How about naming it RuleLifecycleState? :) More fancy and actually it represents the state of the rule. What do you think?
Code quote:
EntryExitKeycomponents/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/parsing/StringSubstitutorNestedMap.java, line 13 at r11 (raw file):
* Version of {@link StringSubstitutor} with the possibility to use a nested Map to define values for the replaced variables. */ public class StringSubstitutorNestedMap extends StringSubstitutor implements StringLookup {
How about MapStringSubsitutor? It's a bit simpler :)
Code quote:
StringSubstitutorNestedMapmove and rename EntryExitKey Enum; refactor ConfigDocsGenerator to Singleton; small formatting fixes
There was a problem hiding this comment.
Reviewable status: 14 of 90 files reviewed, 41 unresolved discussions (waiting on @MariusBrill and @mariusoe)
components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/ConfigDocsGenerator.java, line 30 at r11 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
As far as I see, there is no reason why you should create multiple instances of this class. You just want to use the
generate..method.
In order to rpevent unneccessary object creating, we can use a singleton pattern here.Just create one instance, that will be used everywhere.
public static final ConfigDocsGenerator INSTANCE = new ConfigDocsGenerator(); private ConfigDocsGenerator() { }or
private static final ConfigDocsGenerator INSTANCE = new ConfigDocsGenerator(); public static ConfigDocsGenerator getInstance() { return INSTANCE; } private ConfigDocsGenerator() { }
Done.
As talked about it only gets created once either way anyways and it being a Singleton complicates testing somewhat, so we will keep it as is.
components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/model/MetricDocs.java, line 18 at r11 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Remove the empty line please :)
Done.
components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/model/MetricDocs.java, line 20 at r11 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Imo
nullis better to represent that it does not exists.
Done.
components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/model/RuleDocs.java, line 99 at r11 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
You can move this into a own file because it is used in multiple classes.
Done.
components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/model/RuleDocs.java, line 99 at r11 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
How about naming it
RuleLifecycleState? :) More fancy and actually it represents the state of the rule. What do you think?
Done.
I like that name a lot more, yes :)
components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/parsing/StringSubstitutorNestedMap.java, line 13 at r11 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
How about
MapStringSubsitutor? It's a bit simpler :)
Done.
As talked about the original StringSubstitutor already uses a Map, so I think keeping the Nested is more exact, but it's been renamed to NestedMapStringSubstitutor for better understanding.
Move Yaml Strings back into files
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, 11 of 29 files at r4, 54 of 76 files at r10, 1 of 1 files at r11, 7 of 9 files at r12, 14 of 14 files at r13, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @MariusBrill)
This change is