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

Patrick/tamper #6

Merged
merged 8 commits into from
Oct 19, 2023
Merged

Patrick/tamper #6

merged 8 commits into from
Oct 19, 2023

Conversation

garmr-ulfr
Copy link
Contributor

I didn't implement tamper for UDP as we use solely TCP (as far as I know).

@garmr-ulfr garmr-ulfr requested a review from Crosse October 9, 2023 21:07
@Crosse
Copy link
Contributor

Crosse commented Oct 10, 2023

Man, who thought it was a good idea to turn linting to 11?

checks notes, finds out it was me all along

Oh, yeah, that tracks. 🙄

Copy link
Contributor

@Crosse Crosse left a comment

Choose a reason for hiding this comment

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

This is great! Thank you for buckling down and doing what I never seemed to have time to finish. This could absolutely be merged as-is as long as we ticketed a few of the outstanding items, but if we have the time and you wouldn't mind taking a look at the few comments I left and the things the linter found, I'd appreciate it.

actions/tamper_action.go Outdated Show resolved Hide resolved
actions/tamper_action.go Outdated Show resolved Hide resolved
actions/tamper_action.go Outdated Show resolved Hide resolved
actions/tamper_action.go Outdated Show resolved Hide resolved
actions/tamper_action.go Outdated Show resolved Hide resolved
actions/tamper_action.go Outdated Show resolved Hide resolved
actions/tamper_action.go Outdated Show resolved Hide resolved
actions/tamper_action.go Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Oct 13, 2023

Pull Request Test Coverage Report for Build 6520555836

  • 192 of 371 (51.75%) changed or added relevant lines in 5 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-3.6%) to 58.883%

Changes Missing Coverage Covered Lines Changed/Added Lines %
actions/fragment_packet.go 18 22 81.82%
actions/duplicate_action.go 1 7 14.29%
actions/tamper_action.go 138 307 44.95%
Files with Coverage Reduction New Missed Lines %
actions/tamper_action.go 2 44.29%
Totals Coverage Status
Change from base Build 3038527816: -3.6%
Covered Lines: 822
Relevant Lines: 1396

💛 - Coveralls

actions/duplicate_action.go Outdated Show resolved Hide resolved
actions/tamper_action.go Outdated Show resolved Hide resolved
common/packet.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Crosse Crosse left a comment

Choose a reason for hiding this comment

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

I haven't kept up with Go's recent changes to how it renders documentation (it can do Markdown now, right?), but I don't know that switching from spaces to a mix of tabs and spaces is useful, since it makes those lines highly dependent on how wide someone's tabs are. (Mine are always 8 spaces; VSCode uses four, etc.) I'd also like to remove the memory allocation/copy during checksum calculation, then this will be good to go.

common/packet.go Outdated Show resolved Hide resolved
@garmr-ulfr
Copy link
Contributor Author

I haven't kept up with Go's recent changes to how it renders documentation (it can do Markdown now, right?)

What about Markdown?

but I don't know that switching from spaces to a mix of tabs and spaces is useful, since it makes those lines highly dependent on how wide someone's tabs are

Did tabs get inserted?? I just use the auto format. I did switch from gofmt to gofumpt though. I don't think that should have an affect on my editor. I'll have to check if I messed something up.

@garmr-ulfr garmr-ulfr merged commit 9ffe6f3 into main Oct 19, 2023
2 checks passed
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.

None yet

3 participants