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

Refactor parameter types: change some String parameters to traits and… #61

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

kolapapa
Copy link
Contributor

When constructing the LdapClient using LdapClientBuilder, there is no need for the user to manually construct the URL as a Url type before passing it in. Instead, it should be automatically converted to a Url type by the library during the actual build process, which can enhance the user experience. The same reasoning applies to the basedn in other client query operations.

Copy link
Member

@Firstyear Firstyear left a comment

Choose a reason for hiding this comment

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

I like the into change, but I'd like the URL change reverted. Currently, by having the url as part of clap, it means we get really good errors on the cli like:

error: Invalid value "ldap" for '--url <URL>': relative URL without a base

Where as with this change, we'd only get "InvalidUrl".

It also means callers to our library pass in a Url and can handle the errors there too in a better way than we can.

So can you please revert the URL change?

@kolapapa
Copy link
Contributor Author

I like the into change, but I'd like the URL change reverted. Currently, by having the url as part of clap, it means we get really good errors on the cli like:

error: Invalid value "ldap" for '--url <URL>': relative URL without a base

Where as with this change, we'd only get "InvalidUrl".

It also means callers to our library pass in a Url and can handle the errors there too in a better way than we can.

So can you please revert the URL change?

I think this LdapError needs some optimization. The current displayed error message is too limited and does not allow for personalized output.

@Firstyear
Copy link
Member

That's the output from Clap + Url you're seeing there.

LdapError isn't meant to have deep personalised output for a command line or whatever. It's why we would prefer to have the URL parsed first by the caller who can handle and get better output there.

We do want to improe error handling in future, to make unique errors per raise site, but that's ongoing.

@Firstyear Firstyear merged commit 2972cfd into kanidm:master Oct 31, 2024
1 check passed
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.

2 participants