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

Making client features optional in icann-rdap-common #89

Open
valkum opened this issue Nov 15, 2024 · 2 comments
Open

Making client features optional in icann-rdap-common #89

valkum opened this issue Nov 15, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@valkum
Copy link

valkum commented Nov 15, 2024

Hey, we use structs from the common crate to create some responses in our own RDAP server implementation.

The icann-rdap-common crate depends on some crates (such as reqwest) that are only needed for the client module. Would it be possible to reqwest optional and hide the client module behind a feature flag?

If you agree, I can work on a PR for this change. We can keep it enable by default so that it wouldn't be a potential breaking change.

@anewton1998
Copy link
Collaborator

I think this is a good idea in the short-term. But in the long-term we should move all the reqwest stuff back to icann-rdap-client. This got polluted some how when we introduced bootstrapping to icann-rdap-srv. Are you willing to live with a temporary solution as you have proposed?

BTW, I have some changes for common in the queue to help deal with misbehaving servers. Also, if you are using these structs outside of the binaries of this project, feedback on how to make them better would be welcomed. The API could be better, but I am too close to the problem sometimes.

@valkum
Copy link
Author

valkum commented Nov 27, 2024

There are some things, that look a bit ugly.

  • T::builder could provide better defaults. Removing the need to pass things like Common::new_level0(vec![], vec![]). You can't always use the builder shortcuts such as basic.
  • I think the derived builders produce some unergonomic methods. E.g. common vs. object_common. Handwritten builders probably provide better usability and control.
  • In general, a lot of the response types need a lot of heap allocations for static information.
    • E.g. StatusValue could be an enum with a set of registered values and a fallback Other(String) variant.

I also suggest setting up dependabot if you don't have a fixed update schedule. There are some outdated dependencies that cause dependency duplication downstream. E.g. syn v1 via strum_macros v0.24.

@anewton1998 anewton1998 added the enhancement New feature or request label Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants