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

Emit dns servers in DHCPv4 repr. Fixes #504 #505

Merged
merged 5 commits into from
Jun 27, 2021

Conversation

theli-ua
Copy link
Contributor

No description provided.

@theli-ua
Copy link
Contributor Author

ok, no alloc, right, let me fix that

src/wire/dhcpv4.rs Outdated Show resolved Hide resolved
src/wire/dhcpv4.rs Outdated Show resolved Hide resolved
src/wire/dhcpv4.rs Outdated Show resolved Hide resolved
@theli-ua
Copy link
Contributor Author

@Dirbaio I have a question re source formatting, it is a bit of a pain that one can't just do rustfmt. Is that intended?

@theli-ua theli-ua requested a review from Dirbaio June 23, 2021 19:49
@theli-ua
Copy link
Contributor Author

all clippy warnings are from code thats already there

@crawford
Copy link
Contributor

@theli-ua do you mind fixing the clippy warnings while you are in here? Regarding rustfmt, the maintainers have a preferred style that isn't easily expressible and have opted to format the code by hand.

src/wire/dhcpv4.rs Outdated Show resolved Hide resolved
@theli-ua
Copy link
Contributor Author

manual formatting seem to serves as unnecessary friction and deterrent for potential contributions tbh.

I fixed clippy warnings, but really they were all over the place and have nothing to do with the change in this pr

@crawford
Copy link
Contributor

Hmm, that was a much larger fix than I expected. I only saw a single clippy error from e023164 (though, I realize now you mentioned warnings, not errors). I think you can probably drop that last commit from this PR.

@theli-ua
Copy link
Contributor Author

@crawford I've removed last commit and all checks are passing. Let me know if there is more feedback

@crawford
Copy link
Contributor

Overall, this seems fine to me (might want to squash the commit history in this PR though), but I’m not a maintainer so I must defer to @Dirbaio.

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@Dirbaio Dirbaio merged commit 1d3b3a7 into smoltcp-rs:v0.7.x Jun 27, 2021
@theli-ua theli-ua deleted the v0.7.x branch June 28, 2021 17:25
@theli-ua
Copy link
Contributor Author

@Dirbaio what's the current approach for 0.7.x maintenance releases?

@Dirbaio
Copy link
Member

Dirbaio commented Jun 28, 2021

If you want, send a PR against the v0.7.x branch cherry-picking this change, and I'll release it.

@theli-ua
Copy link
Contributor Author

@Dirbaio 0.7.x is the branch it was merged into :D

@Dirbaio
Copy link
Member

Dirbaio commented Jun 28, 2021

Oh! 🤣

Okay, let me release and cherry-pick into master.

@Dirbaio
Copy link
Member

Dirbaio commented Jun 28, 2021

v0.7.5 now out

@Dirbaio
Copy link
Member

Dirbaio commented Jun 28, 2021

@theli-ua next time send PRs to master, we later cherry-pick cherrypickable features back to v0.7.x, not the other way around.

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.

3 participants