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

Redesign to version v3 #108

Merged
merged 30 commits into from
Apr 13, 2023
Merged

Redesign to version v3 #108

merged 30 commits into from
Apr 13, 2023

Conversation

elivlo
Copy link
Collaborator

@elivlo elivlo commented Feb 23, 2023

I have done some brainstorming on how to redesign this library.
Here are my first ideas on how we could change the API:

  • I thought that all the different Run functions were too much
    • So I broke it down to one Run function
  • RunAsync, RunWithProgress, RunWithStreamer now needs to be configured before the run. That also includes two new functions:
    • ToFile: writes XML directly to a file and outputs normal output to stdout
    • Context: adds a context directly to exec command instead of triggering the timeout in code
    • Async, Progress and Streamer are the derived functions
  • This uses a kinda like Builder design pattern

@Ullaakut What do you think of this idea?

@elivlo elivlo added enhancement New feature or request help wanted Extra attention is needed labels Feb 23, 2023
@elivlo elivlo added this to the Redesign to version v3 milestone Feb 23, 2023
@elivlo elivlo requested a review from Ullaakut February 23, 2023 11:32
@elivlo elivlo self-assigned this Feb 23, 2023
@elivlo elivlo marked this pull request as ready for review February 24, 2023 12:01
@elivlo elivlo marked this pull request as draft February 24, 2023 12:26
Copy link
Owner

@Ullaakut Ullaakut left a comment

Choose a reason for hiding this comment

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

Great work! I think this makes the project much much cleaner and easier to build upon. I have a few suggestions as to how it can be improved even slightly further IMO, as well as some nitpicks.

Thanks again @elivlo ! If you setup a GitHub sponsors account one day I'd be glad to give you some money for your invaluable help on this project.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
examples/basic_scan/main.go Outdated Show resolved Hide resolved
examples/basic_scan_progress/main.go Show resolved Hide resolved
examples/basic_scan_progress/main.go Outdated Show resolved Hide resolved
nmap.go Outdated Show resolved Hide resolved
nmap.go Outdated Show resolved Hide resolved
nmap.go Outdated Show resolved Hide resolved
nmap.go Outdated Show resolved Hide resolved
nmap.go Outdated Show resolved Hide resolved
@elivlo elivlo force-pushed the dev/v3 branch 3 times, most recently from 3e9d564 to 00501bf Compare April 11, 2023 16:36
@elivlo elivlo force-pushed the dev/v3 branch 2 times, most recently from 3846753 to 418a8e0 Compare April 11, 2023 17:21
@elivlo
Copy link
Collaborator Author

elivlo commented Apr 11, 2023

@Ullaakut I finished all my work on this branch :)
Would you please take a last look at it before I merge it to master?

@elivlo elivlo marked this pull request as ready for review April 11, 2023 17:24
@elivlo
Copy link
Collaborator Author

elivlo commented Apr 11, 2023

@Ullaakut I finished all my work on this branch :) Would you please take a last look at it before I merge it to master?

Oh, I forgot some hidden comments.

Copy link
Owner

@Ullaakut Ullaakut left a comment

Choose a reason for hiding this comment

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

Some more nitpicks, but this looks good to me :)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
examples/basic_scan/main.go Outdated Show resolved Hide resolved
examples/basic_scan_async/main.go Outdated Show resolved Hide resolved
examples/basic_scan_streamer_interface/main.go Outdated Show resolved Hide resolved
go.mod Outdated
Comment on lines 12 to 13
github.com/davecgh/go-spew v1.1.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
Copy link
Owner

Choose a reason for hiding this comment

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

For sure those two should not be added. Not sure about the YAML dependency as well but I didn't read everything yet

go.mod Outdated Show resolved Hide resolved
optionsPortOrder.go Outdated Show resolved Hide resolved
optionsPortOrder.go Outdated Show resolved Hide resolved
@elivlo elivlo force-pushed the dev/v3 branch 3 times, most recently from fccf508 to a96b94e Compare April 13, 2023 20:31
@elivlo elivlo merged commit 0a4c3dc into master Apr 13, 2023
@elivlo elivlo deleted the dev/v3 branch April 13, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants