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

Eli 47 transfer link inspector settings to granite console #62

Merged

Conversation

prosis6
Copy link

@prosis6 prosis6 commented Sep 6, 2024

#52
#47 - implemented page with configs (load & save working automatically). available on /apps/etoolbox-link-
inspector/options/options.html. Needed implementation of internal integration with Link Inspector logic.
#37
#35

Saldatsenka, Siarhei added 3 commits August 6, 2024 10:56
…-granite-console

# Conflicts:
#	core/src/main/java/com/exadel/etoolbox/linkinspector/core/services/resolvers/ExternalLinkResolverImpl.java
#	core/src/main/java/com/exadel/etoolbox/linkinspector/core/services/resolvers/InternalLinkResolverImpl.java
…t' console. #47

-added missed fields to options page
-fixed css styles
@@ -68,4 +72,21 @@ public static void writeJsonResponse(SlingHttpServletResponse response, String j
LOG.error("Failed to write json to response", e);
}
}

public static Resource createTab(ResourceResolver resolver, String path, String title, Collection<Resource> children) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The methods are allright but I don't think they belong to ServletUtil. ServletUtil is exactly abound manipulating Request and Response. And these methods are about creating Synthetic resources. This is a different thing. They should be moved to another utility class.

I believe this is is going to be LinkInspectorResourceUtil.
Besides, the name of LinkInspectorResourceUtil should be cut down to ResourceUtil. The whole project is "Link Inspector" all around -- there is no need to repeat that in the name of a particular class.

Another option -- and maybe a better one -- is to create a new GraniteUtil class. That's because these are not just generic resource-creatign methods. They create a very special type of resources -- the ones for the Granite Datasource technology

Copy link
Author

Choose a reason for hiding this comment

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

moved to GraniteUtil class

@@ -0,0 +1,114 @@
package com.exadel.etoolbox.linkinspector.core.servlets;

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

(cosmetic) Please observe this notice is put before the imports section here , but the same one is put after the imports section in the previous class. Need to put them in the same place,

Do we need these notices at all? No objection from my side, but:

  • this is not customary for the project. Should all the other people start adding theirs in all the other classes?
  • the authorship is clearly seen from GIT history anyway

Copy link
Author

Choose a reason for hiding this comment

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

removed


private Map<String, List<AttributeDefinition>> getServiceSettings() {
Map<String, List<AttributeDefinition>> result = new HashMap<>();
for (LinkResolver linkResolver : linkResolvers) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

linkResolvers can be null so it is better to wrap in CollectionUtils.emptyIfNull()

Copy link
Author

Choose a reason for hiding this comment

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

thanks, added NPE check

- deleted 'Exclude tags' check box on options page
@prosis6 prosis6 merged commit 100236e into develop Oct 3, 2024
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.

3 participants