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

Update Rust guidelines for module layout #8490

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

heaths
Copy link
Member

@heaths heaths commented Feb 5, 2025

No description provided.


Rust modules should be defined such that:

1. All clients and models that the user can create are exported from the crate root e.g., `azure_security_keyvault_secrets`.
Copy link
Member

Choose a reason for hiding this comment

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

I worry that this makes the root a bit cluttered. Maybe it's ok for clients/models/enum types but for client method options do we think there's value in having them in the root? More often than not callers just pass None (at least this is the case in Go). What if we were to either leave them in clients only, or put them in a separate module?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to it. The idea was to have all creatable in the root, but there's likely more client method options than request models. Well, unless the request models include a lot of sub-models like Cognitive would have. Hmm, now I'm starting to rethink this. Maybe we shouldn't export models from the root.

@RickWinter @JeffreyRichter what do you think? I mean, rust-analyzer should make light work of these either way and it does mitigate possibly shuffling models in an out (edge case, but definitely happens).

So, it'd be:

  • Creatable clients, and their client options and client method options, in the root (but re-exported from clients just so all clients are always found there).
  • Subclients, and their client options and client method options, in the clients submodule only.
  • All models only from models.

@analogrelay and @vincenttran-msft how do you feel about this?

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's cleaner.

I do still have reservations about client method options being in the root. What if we put them in the root IFF they contain modeled fields? I think it would help quiet down the noise, unless we expect callers to do something with the method_options field on a fairly routine basis.

Copy link
Member Author

@heaths heaths Feb 5, 2025

Choose a reason for hiding this comment

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

Maybe, but I'm not too crazy about the subtleties here. I doubt customers would get the subtlety and I imagine I'll probably even forget after a time why some are re-exported from the root and others aren't.

What does Go do here? Or, IIRC, does it export everything from the root which, yes, is noisy. I do want to be less noisy, but also not introduce a bunch of subtlety that would likely be hard to maintain for various reasons and likely lost on customers anyway.

That said, but the reasons you brought up, maybe all client method options are only exported from clients. I think we still keep client options with their clients as those will likely get used more.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, client options stay with their client (e.g. for instantiable clients, its client options type would also go in the root).

Agreed on the subtleties. Keeping the client method options in clients is an improvement IMO since so many of them are essentially empty. And if customers hate it, we can always change it later before GA.

In Go we dump everything in the root for two reasons.

  • to avoid import explosions
  • to avoid sub-package import name collisions

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point. However, I think the same applies if we are to keep them in the clients module. With that being the case, do we like foo_crate::clients::FooClientGetOptions or foo_crate::options::FooClientGet better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer the latter just because it's clear. By default, rust-analyzer is going to import types so you eng up with FooClientGet which is not clear on face value what it does. "Options" being part of the name hopefully seems more obvious. It's also idiomatic: std, tokio, and many others have "***Options" types.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, let's keep the Options suffix. So, do we want to keep them all in the clients module then? Or do we want to put all client method options in some other module?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think client options should be exported from clients but client method options should / could be exported from models. In a way, that's really what they are and I don't want to add a bunch of modules unless it makes a lot of sense to.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I like this, let's start with that.

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.

5 participants