Skip to content

Conversation

@gbhat618
Copy link

@gbhat618 gbhat618 commented Jan 20, 2025

Currently converting a given tree specification into the NamedPathPruner.Tree parsing logic is present only the NamedPathPruner class and not exposed to outside. The parsed value tree and it's children are also not exposed outside (imo, it is done from an encapsulation standpoint, which is correct).

I have a use cases, I would like to parse the give tree spec, just need the list of keys. Although it would be possible

  • either - write a usecase specific parsing logic based on delimiters like , [ ], but it will lead to logical mismatch between the parsing logic here and the one implemented for a specific use case.
  • or - use reflection and access the children of the tree, it is not good.

To expose the parsing logic, merely changing the access specifier of methods related to parsing (parse, list, node etc.) or exposing the parsed value by getter - getTree or getChildren would break the encapsulation and may lead to problems in the downstream code.

This PR proposes to add a utility method without modifying any access specifier or changing state related logic.

I have considered alternatives such as,

  • adding a new utility class such as NamedPathPrunerUtil to expose the functionality
  • and, trying to extend the into child class (but needs to make the NamedPathPruner non-final)

but this proposed solution seems better one.

Testing done

  • Added automated test, ensured it is running and covers 100% line coverage for the new code.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

* @param spec the textual specification
* @return a hierarchical tree represented as {@code TreeMap<String, Object>}.
*/
public static TreeMap<String, Object> parseAsTreeMap(String spec) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this cannot just be typed as a Map?

* Each node is represented as a key-value pair in the returned {@code TreeMap}.
* The value for a key:
* <ul>
* <li>Is another {@code TreeMap<String, Object>} for non-leaf nodes.</li>
Copy link
Member

Choose a reason for hiding this comment

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

This requires the caller to cast, and even worse to use unchecked conversions. I think it would be better to expose Tree or some interface it implements.

Copy link
Member

Choose a reason for hiding this comment

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

minimally

interface Something extends Map<String, Something> {}

might work

@jglick
Copy link
Member

jglick commented Apr 28, 2025

Note that the current API does not expose the range. Not sure if anyone would care about this anyway, but worth mentioning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants