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

[Feature] Group unhandled paths #91

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Filipoliko
Copy link
Contributor

@stephenhillier I started working on the update discussed in #82 and I am going with __unknown__ value in the end as the _ symbol is unlikely to conflict with some actual api endpoint.

As I was finishing up the changes, it seemed quite impractical to support both filter_unhandled_paths and group_unhandled_paths options as only one of these is applicable. It leads to weird situations like, if you want to enable group_unhandled_paths you also need to explicitly disable filter_unhandled_paths as it is enabled by default. Maybe 2 different flags are not the right way to go? Maybe an option like unhandled_paths_strategy accepting string values ignore (replacement for filter_unhandled_paths), group (replacement for group_unhandled_paths), or unique (replacement for setting both filter_unhandled_paths and group_unhandled_paths to False, but not sure about the name unique) to decide how to handle these situations. I think this would be a better way to go, but if we remove the filter_unhandled_paths, it will be a breaking change.

If we go with unhandled_paths_strategy option, the default value could be actually based on group_paths option. If enabled, the default value would be group, if disabled, default value could be unique.

If we go with group_unhandled_paths option, what should happen if both filter_unhandled_paths and group_unhandled_paths are set to True? I threw an error, but I am not sure if thats the right way to go.

I pushed the original solution as a draft and we can discuss which direction we should go.

Going with a new flag would be least disruptive to users as it does not introduce any breaking changes, but seems quite unintuitive when setting up the configuration (disabling one flag to be able to enable another one).

@stephenhillier
Copy link
Owner

As I was finishing up the changes, it seemed quite impractical to support both filter_unhandled_paths and group_unhandled_paths options as only one of these is applicable.

I see what you mean. I think it's ok for group_unhandled_paths to simply override filter_unhandled_paths, especially now that the latter defaults to true (users no longer need to set it except to disable it)? If a user sets group_unhandled_paths, the intent is clear enough that there wouldn't be any confusion even if filter_unhandled_paths is left as True. Thoughts?

@atos-piotr-hyzy
Copy link

filter_unhandled_paths

As a user of the exporter, I would prefer that group_unhandled_paths be the default behavior when filter_unhandled_paths is set to true. This simplifies the configuration and ensures clear intent without confusion. Allowing for mutually exclusive options and validating them during app bootstrapping might complicate things unnecessarily. Therefore, I would link group_unhandled_paths with filter_unhandled_paths and not allow them to be changed separately. Optionally, I would expose the possibility to set the name of the representation of an "unhandled" path.

stephenhillier added a commit that referenced this pull request Jun 23, 2024
[Feature] Add `group_unhandled_paths` Option to Enhance `filter_unhandled_paths` Functionality  #91
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.

None yet

3 participants