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(pedm): cleanup and clippy #1288

Merged
merged 7 commits into from
Mar 27, 2025
Merged

refactor(pedm): cleanup and clippy #1288

merged 7 commits into from
Mar 27, 2025

Conversation

allan2
Copy link
Contributor

@allan2 allan2 commented Mar 26, 2025

This resolves all clippy errors in the the devolutions-pedm crate.

allan2 added 5 commits March 26, 2025 11:42
We control usage and there is more refractoring to come. `pub` is simpler.
Modules are now `pub` to complete the change.
Copy link

Let maintainers know that an action is required on their side

  • Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)

  • Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release

  • Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)

  • Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

pub(crate) async fn post_elevate_session(
pub async fn post_elevate_session(
Copy link
Member

@CBenoit CBenoit Mar 26, 2025

Choose a reason for hiding this comment

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

question: Why did you make everything public? Is it API we actually intended to expose from this crate? I believe not, because, for instance, this function is an HTTP handler. I would rather not increase the API surface of any crate more than necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it is simpler, because we control usage. There are no consumers of this crate at the moment. If you prefer, I can undo this commit to keep pub(crate).

Copy link
Member

@CBenoit CBenoit Mar 27, 2025

Choose a reason for hiding this comment

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

I understand where you come from, but the problem is that there is no dead_code lint across crate boundaries. We are much more likely to keep around dead code if it’s publicly exposed from the crate. I also tend to prefer clear pub(crate) over pub when the item is never used outside of the crate, just so I have a clear idea on what is an API boundary (even if we are the only ones to use it).
That being said, the workspace unused pub items problem could be addressed with a tool such as https://github.com/cpg314/cargo-workspace-unused-pub
If we integrate it in our CI, this may become mostly a non issue (besides my preference for clearly marking a pub(crate) item as pub(crate)).

@allan2 allan2 requested review from a team as code owners March 27, 2025 13:41
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

LGTM!

@CBenoit CBenoit enabled auto-merge (squash) March 27, 2025 14:11
@CBenoit CBenoit merged commit 66422a4 into master Mar 27, 2025
77 checks passed
@CBenoit CBenoit deleted the pedm-libsql branch March 27, 2025 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants