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

Update msfvenom #5027

Closed
wants to merge 1 commit into from
Closed

Update msfvenom #5027

wants to merge 1 commit into from

Conversation

DrDinosaur
Copy link

Grammar and style.

Grammar and style.
@OJ OJ mentioned this pull request Mar 29, 2015
@hdm
Copy link
Contributor

hdm commented Mar 30, 2015

Hi @DrDinosaur. As a rule we don't enforce punctuation on comments and we try to keep the line length of printed options smaller even at the cost of grammar. Some of these cosmetic updates look useful (comment sentence capitalization is nice), but others go against the intent (smaller line length in help output). Related are #5028 & #5026

@@ -117,7 +117,7 @@ require 'msf/core/payload_generator'
opts[:keep] = true
end

opt.on('--payload-options', "List the payload's standard options") do
opt.on('--payload-options', 'List the payload's standard options') do
Copy link

Choose a reason for hiding this comment

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

Given the interior ' this will break formatting.

@todb-r7
Copy link

todb-r7 commented Mar 30, 2015

+1 to HD's comments -- I don't expect to spend a lot of effort fixing punctuation and caps in inline comments, especially non-YARD formatted comments, since it'll never really be consumed by an API reference doc or anything.

I expect to pull in a couple of these changes, just trying to figure out the best/easiest way to land only a little of this PR with some creative merging, reverting, and --author rewriting. :)

I like #5026 and #5028 though. Will review those as well.

@todb-r7 todb-r7 self-assigned this Mar 30, 2015
todb-r7 pushed a commit to todb-r7/metasploit-framework that referenced this pull request Mar 30, 2015
Switching from " to ' is usually more trouble than it's worth, even if
it's more technically correct. The original PR had a great example of
that kind of error, where you accidentally screw up an interior
apostraphe.

[See rapid7#5027]
@todb-r7 todb-r7 mentioned this pull request Mar 30, 2015
@todb-r7 todb-r7 closed this in b770f8d Mar 30, 2015
@todb-r7
Copy link

todb-r7 commented Mar 30, 2015

Note that the merged version now conflicts with this branch, @DrDinosaur you're free to delete the branch on your side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants