Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes #1278 - Add customized syntax highlighting for YAML editor in config-server #1282

Conversation

aaronweissler
Copy link
Member

@aaronweissler aaronweissler commented Jan 24, 2022

This change is Reviewable

@aaronweissler aaronweissler linked an issue Jan 24, 2022 that may be closed by this pull request
@aaronweissler aaronweissler force-pushed the feature/yaml-editor-syntax-highlighting branch from bcb16f4 to b4b2d0d Compare January 24, 2022 14:50
@aaronweissler aaronweissler force-pushed the feature/yaml-editor-syntax-highlighting branch from b4b2d0d to f456ec2 Compare January 26, 2022 11:11
…nd tests added in HighlightingRulesMapControllerIntTest
Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 5 files at r1, 2 of 3 files at r2, all commit messages.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @aaronweissler and @heiko-holz)


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/yamlhighlighter/HighlightRulesMapController.java, line 93 at r1 (raw file):

    @ApiOperation(value = "Get JSON for Highlighting Rules Generation", notes = "")
    @GetMapping(value = "highlightRulesMap", produces = MediaType.APPLICATION_JSON_VALUE)
    public Map<String, Object> getHighlightRulesMap() {

Testing would be very good.

I have committed a test for getHighlightRulesMap in the HighlightRulesMapControllerIntTest.java


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/yamlhighlighter/HighlightRulesMapController.java, line 148 at r2 (raw file):

    @ApiOperation(value = "Get JSON for Highlight Rules Generation", notes = "")
    @GetMapping(value = "highlight-rules", produces = MediaType.APPLICATION_JSON_VALUE)
    public Map<String, Object> getHighlightRulesMap() {

In general, we were thinking of not returning the nested Map here but just the result of generateMap(InspectitConfig.class) and let the JavaScript code rebuild the object that Ace needs.


components/inspectit-ocelot-configurationserver-ui/src/components/editor/yaml-editor/InspectitOcelotHighlightRules.js, line 375 at r1 (raw file):

    let map_content_type = inner_map.get('map-content-type');

    if (map_content_type === 'object' || map_content_type === 'yaml') {

I would recommend constants instead of inline strings (same applies for other uses of 'object' etc.).

Maybe alsoswitch statements instead of if, elseif

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aaronweissler)

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aaronweissler)


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/yamlhighlighter/HighlightRulesMapController.java, line 76 at r2 (raw file):

    @VisibleForTesting
    static Map<String, Object> generateMap(Class<?> currentClass) {

consider making this method non-static, as it it only used within this class.

Add constants for keys.
Add switch statement instead of if-else
Add support for Object in List
generify List and Map part of generateMap
Support enums outside of inspectIT.config classes
Copy link
Member Author

@aaronweissler aaronweissler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @heiko-holz)


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/yamlhighlighter/HighlightRulesMapController.java, line 93 at r1 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Testing would be very good.

I have committed a test for getHighlightRulesMap in the HighlightRulesMapControllerIntTest.java

OK
I added tests for the generateMap method.


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/yamlhighlighter/HighlightRulesMapController.java, line 76 at r2 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

consider making this method non-static, as it it only used within this class.

Done.


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/yamlhighlighter/HighlightRulesMapController.java, line 148 at r2 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

In general, we were thinking of not returning the nested Map here but just the result of generateMap(InspectitConfig.class) and let the JavaScript code rebuild the object that Ace needs.

Done.
The nested Map is now built in InspectitOcelotHighlightRules.js instead. Definitely helps with readability :)


components/inspectit-ocelot-configurationserver-ui/src/components/editor/yaml-editor/InspectitOcelotHighlightRules.js, line 375 at r1 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

I would recommend constants instead of inline strings (same applies for other uses of 'object' etc.).

Maybe alsoswitch statements instead of if, elseif

Done.

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aaronweissler)

@heiko-holz heiko-holz merged commit 70c359a into inspectIT:master Feb 7, 2022
@aaronweissler aaronweissler deleted the feature/yaml-editor-syntax-highlighting branch June 7, 2022 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customized Syntax Highlighting in config-server's editor
2 participants