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

Add print and return args, fix #86 #87

Merged
merged 3 commits into from
Jul 14, 2021

Conversation

cyrinux
Copy link
Contributor

@cyrinux cyrinux commented Jul 14, 2021

Hi @Ullaakut

This should add what I miss in the issue #86

Regards

cyrinux added 3 commits July 14, 2021 10:29
This should add what I miss in the issue Ullaakut#86

Regards
Rename ReturnArgs() as Args()
@cyrinux
Copy link
Contributor Author

cyrinux commented Jul 14, 2021

Do you

I do not think that a package that is used as an nmap wrapper library should be able to print things. The Args() method is enough, and allows the consumer of the package to do fmt.Println(scanner.Args()) if they want to.

Because of this, I would suggest removing this PrintArgs method altogether.

So should I return a string or an []string?

@Ullaakut
Copy link
Owner

@cyrinux It's better to return the []args and let the consumer of the package decide how they want to print it :)

@cyrinux
Copy link
Contributor Author

cyrinux commented Jul 14, 2021

Ok so we are ok like this 😸

@Ullaakut
Copy link
Owner

@cyrinux Indeed 😄 Thank you for your contribution! :)

@Ullaakut Ullaakut merged commit 6808659 into Ullaakut:master Jul 14, 2021
@Ullaakut Ullaakut added the enhancement New feature or request label Jul 14, 2021
@cyrinux
Copy link
Contributor Author

cyrinux commented Jul 14, 2021

Thanks you @Ullaakut !

@cyrinux
Copy link
Contributor Author

cyrinux commented Jul 14, 2021

Can you cut a release then 🙏🏻 ? I would like to got confidence fix and this one awesome fix 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants