Skip to content

Conversation

@nnethercote
Copy link
Contributor

Things I noticed when reading over the code.

  • Conversions to more idiomatic code.
  • Spelling fixes.
  • Minor comment improvements.
  • Some minor formatting changes.

Things I noticed when reading over the code.

- Conversions to more idiomatic code.
- Spelling fixes.
- Minor comment improvements.
- Some minor formatting changes.

Signed-off-by: Nicholas Nethercote <[email protected]>
@nnethercote
Copy link
Contributor Author

Apologies for the unconnected changes all dumped in a single commit. Lots of opinions here, I'm happy to undo any that rankle. The code quality across the whole repository is generally very good, based on the skimming that I did.

&mut self,
mut comm: R,
req: &PldmRequest<'_>,
host: &mut impl Host,
Copy link
Member

Choose a reason for hiding this comment

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

So we currently have this under the assumption that anything more complex than the current (ie., very basic) file host implementation would need interactions with the host at least for DfOpen operations.

I am happy to remove it now, but we'd likely need it back shortly - which would be an API-breaking change. That said, that change would also modify the Host trait, so we'd have breakages there anyway.

Removing the host arg from the cmd_ handlers is fine though.

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 haven't removed it, precisely because it's public. Changing from host to _host is just needed to prevent the compiler complaining about the unused arg.

Copy link
Member

Choose a reason for hiding this comment

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

hah, I had been focusing all on the - side of the series. All good then!

@mkj mkj merged commit 291f562 into CodeConstruct:main Aug 19, 2025
4 checks passed
@mkj
Copy link
Member

mkj commented Aug 19, 2025

Thanks for those

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.

3 participants