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

formula_desc_cop: ensure no full stops at the end of desc #2795

Merged
merged 1 commit into from Oct 29, 2017

Conversation

Projects
None yet
7 participants
@issyl0
Contributor

issyl0 commented Jun 17, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

  • This was a suggestion by Mike McQuaid in my homebrew-core audit
    description PR. Currently, no formulae do have full stops at the
    end of descriptions (based on ack "desc\b" *.rb | ack "\.$" *.rb),
    but in the interests of future-proofing and consistency, we should
    check for this anyway?
@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Jun 19, 2017

Contributor

It turns out I got my regex wrong for this (7:30am isn't the best time to be thinking about that), so this isn't entirely speculative. Thanks to @ilovezfs for pointing that out in Homebrew/homebrew-core#14229 (comment). I've changed the regex to be correct now, as desc strings end in ", not just full stops.

Contributor

issyl0 commented Jun 19, 2017

It turns out I got my regex wrong for this (7:30am isn't the best time to be thinking about that), so this isn't entirely speculative. Thanks to @ilovezfs for pointing that out in Homebrew/homebrew-core#14229 (comment). I've changed the regex to be correct now, as desc strings end in ", not just full stops.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Jun 24, 2017

Member

@issyl0 Check out the failing brew tests and brew style.

Member

MikeMcQuaid commented Jun 24, 2017

@issyl0 Check out the failing brew tests and brew style.

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Jun 24, 2017

Contributor

@MikeMcQuaid Will do. On Tuesday when I'm back from holiday, if that's OK? Sorry that this is taking so long. I can close and reopen at a later date if you'd like?

Contributor

issyl0 commented Jun 24, 2017

@MikeMcQuaid Will do. On Tuesday when I'm back from holiday, if that's OK? Sorry that this is taking so long. I can close and reopen at a later date if you'd like?

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Jun 24, 2017

Member

On Tuesday when I'm back from holiday, if that's OK?

NO YOU MUST HOMEBREW ALL HOLIDAY

😉

(Yes, of course, totally fine. Just had this in my queue to try and help so figured I'd jump in and offer some advice. Enjoy the holiday!)

Member

MikeMcQuaid commented Jun 24, 2017

On Tuesday when I'm back from holiday, if that's OK?

NO YOU MUST HOMEBREW ALL HOLIDAY

😉

(Yes, of course, totally fine. Just had this in my queue to try and help so figured I'd jump in and offer some advice. Enjoy the holiday!)

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Jun 25, 2017

Contributor

I found a bit of downtime. 👍

I don't know why my editor inserted a weird type of space (it looked like a space, but it wasn't a space), but that's fixed now.

As for the failing test, I don't understand it. No matter how many puts I put around the test case (for expected, actual, etc), I can't see where it's going wrong. I even added ' as well as " to the regex in case it was that, but actual is still nil as it's not detecting a problem. It should be working. I wonder what silly thing I've done wrong or differently this time? It's probably something painfully obvious. Do you know, @MikeMcQuaid?

Contributor

issyl0 commented Jun 25, 2017

I found a bit of downtime. 👍

I don't know why my editor inserted a weird type of space (it looked like a space, but it wasn't a space), but that's fixed now.

As for the failing test, I don't understand it. No matter how many puts I put around the test case (for expected, actual, etc), I can't see where it's going wrong. I even added ' as well as " to the regex in case it was that, but actual is still nil as it's not detecting a problem. It should be working. I wonder what silly thing I've done wrong or differently this time? It's probably something painfully obvious. Do you know, @MikeMcQuaid?

@GauthamGoli

This comment has been minimized.

Show comment
Hide comment
@GauthamGoli

GauthamGoli Jun 25, 2017

Member

@issyl0 You don't have to match ' or " because regex_match_group method matches the string part without quotes. You din't do anything wrong, except that this has to do with the ruby coding guidelines. Your code on lines 81-82 isn't getting executed due to return unless on line 77.

Convert line 77-78 into an if .. block, and also update your regex pattern for full stop matching( remove match for '/" ) . Tests should then pass 👍

Member

GauthamGoli commented Jun 25, 2017

@issyl0 You don't have to match ' or " because regex_match_group method matches the string part without quotes. You din't do anything wrong, except that this has to do with the ruby coding guidelines. Your code on lines 81-82 isn't getting executed due to return unless on line 77.

Convert line 77-78 into an if .. block, and also update your regex pattern for full stop matching( remove match for '/" ) . Tests should then pass 👍

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Jun 27, 2017

Contributor

Aha! Thanks, @GauthamGoli - I must have got confused with the syntax of the test above as that used return unless yet still worked. Thanks for setting me on the right track again with regex_match_group, too - I was overthinking the quotes again!

It doesn't seem to be working again yet though, and I've made the correct changes, I think.

Contributor

issyl0 commented Jun 27, 2017

Aha! Thanks, @GauthamGoli - I must have got confused with the syntax of the test above as that used return unless yet still worked. Thanks for setting me on the right track again with regex_match_group, too - I was overthinking the quotes again!

It doesn't seem to be working again yet though, and I've made the correct changes, I think.

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Jun 27, 2017

Contributor

Sorry this is taking so long and turning complicated!

Contributor

issyl0 commented Jun 27, 2017

Sorry this is taking so long and turning complicated!

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Jun 29, 2017

Member

@issyl0 Never apologise for 1) trying when others don't and 2) learning publicly 👍 👏 ❤️

Member

MikeMcQuaid commented Jun 29, 2017

@issyl0 Never apologise for 1) trying when others don't and 2) learning publicly 👍 👏 ❤️

@GauthamGoli

This comment has been minimized.

Show comment
Hide comment
@GauthamGoli

GauthamGoli Jul 3, 2017

Member

@issyl0 Probably I told it wrong, but I meant, Line 77 - 78 needs to be in if-end block while Line 81 -83 has be return unless guard clause, not vice versa. Otherwise Line 81 -83 may not get executed because there is a return unless statement on line 77.

Member

GauthamGoli commented Jul 3, 2017

@issyl0 Probably I told it wrong, but I meant, Line 77 - 78 needs to be in if-end block while Line 81 -83 has be return unless guard clause, not vice versa. Otherwise Line 81 -83 may not get executed because there is a return unless statement on line 77.

@JCount

This comment has been minimized.

Show comment
Hide comment
@JCount

JCount Jul 3, 2017

Member

@issyl0 Feel free to rebase my commit, I thought I would just resolve the conflict since I was looking at this PR anyway. 😄

Member

JCount commented Jul 3, 2017

@issyl0 Feel free to rebase my commit, I thought I would just resolve the conflict since I was looking at this PR anyway. 😄

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Jul 7, 2017

Member

@issyl0 Shout if we can help here at all but: no rush (and hope you had a good holiday)!

Member

MikeMcQuaid commented Jul 7, 2017

@issyl0 Shout if we can help here at all but: no rush (and hope you had a good holiday)!

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Jul 8, 2017

Contributor

@JCount Thanks for that, but I did just rebase out your merge commit. :-)

Contributor

issyl0 commented Jul 8, 2017

@JCount Thanks for that, but I did just rebase out your merge commit. :-)

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Jul 8, 2017

Contributor

@GauthamGoli Thanks! That makes a lot more sense, and now I can see the problem. I've just made that change and tests pass locally.

@MikeMcQuaid I did have a good holiday, thanks! The tests pass for this now, apart from brew style which is complaining about ifs instead of returns again: Use a guard clause instead of wrapping the code inside a conditional expression.

Contributor

issyl0 commented Jul 8, 2017

@GauthamGoli Thanks! That makes a lot more sense, and now I can see the problem. I've just made that change and tests pass locally.

@MikeMcQuaid I did have a good holiday, thanks! The tests pass for this now, apart from brew style which is complaining about ifs instead of returns again: Use a guard clause instead of wrapping the code inside a conditional expression.

@stale stale bot added the stale label Jul 30, 2017

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Jul 30, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale bot commented Jul 30, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot closed this Aug 12, 2017

@DomT4 DomT4 referenced this pull request Oct 23, 2017

Closed

singular 4.1.0p3 (new formula) #19474

4 of 4 tasks complete
@DomT4

This comment has been minimized.

Show comment
Hide comment
@DomT4

DomT4 Oct 23, 2017

Contributor

What happened here? The PR looks to be in pretty good shape but it was closed as stale in the end. Was it just the guard clause hiccup?

Contributor

DomT4 commented Oct 23, 2017

What happened here? The PR looks to be in pretty good shape but it was closed as stale in the end. Was it just the guard clause hiccup?

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Oct 23, 2017

Contributor

@DomT4 That was it. And then I disappeared off the face of the planet, apparently. :-)

Contributor

issyl0 commented Oct 23, 2017

@DomT4 That was it. And then I disappeared off the face of the planet, apparently. :-)

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 23, 2017

Contributor

@issyl0 should we reopen it?

Contributor

ilovezfs commented Oct 23, 2017

@issyl0 should we reopen it?

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Oct 23, 2017

Contributor

@ilovezfs If you'd like. I have time to work on it again I think (this week), and it's literally one thing that's wrong. But I don't remember if I ever knew how to fix it.

Contributor

issyl0 commented Oct 23, 2017

@ilovezfs If you'd like. I have time to work on it again I think (this week), and it's literally one thing that's wrong. But I don't remember if I ever knew how to fix it.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 23, 2017

Contributor

@issyl0 it will also need to be made consistent with #3349 but that shouldn't be a huge deal.

Contributor

ilovezfs commented Oct 23, 2017

@issyl0 it will also need to be made consistent with #3349 but that shouldn't be a huge deal.

@ilovezfs ilovezfs reopened this Oct 23, 2017

@stale stale bot removed the stale label Oct 23, 2017

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 26, 2017

Contributor

@issyl0 #3349 has now been merged. Please rebase this PR on master when you have a chance! Thanks :)

Contributor

ilovezfs commented Oct 26, 2017

@issyl0 #3349 has now been merged. Please rebase this PR on master when you have a chance! Thanks :)

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 28, 2017

Contributor

@issyl0

diff --git a/Library/Homebrew/rubocops/formula_desc_cop.rb b/Library/Homebrew/rubocops/formula_desc_cop.rb
index 4233c3a..bc36330 100644
--- a/Library/Homebrew/rubocops/formula_desc_cop.rb
+++ b/Library/Homebrew/rubocops/formula_desc_cop.rb
@@ -84,9 +84,8 @@ module RuboCop
           end
 
           # Check if a full stop is used at the end of a formula's desc
-          if regex_match_group(desc, /.*\.$/)
-            problem "Description shouldn't end with a full stop"
-          end
+          return unless regex_match_group(desc, /\.$/)
+          problem "Description shouldn't end with a full stop"
         end
 
         private
diff --git a/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb b/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb
index c82a1f2..aacb52e 100644
--- a/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb
+++ b/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb
@@ -96,9 +96,8 @@ describe RuboCop::Cop::FormulaAuditStrict::Desc do
         class Foo < Formula
           url 'http://example.com/foo-1.0.tgz'
           desc 'Description with a full stop at the end.'
-                                                       ^ Description shouldn\'t end with a full stop.
-          end
-      end
+                                                       ^ Description shouldn\'t end with a full stop
+        end
       RUBY
     end
 
Contributor

ilovezfs commented Oct 28, 2017

@issyl0

diff --git a/Library/Homebrew/rubocops/formula_desc_cop.rb b/Library/Homebrew/rubocops/formula_desc_cop.rb
index 4233c3a..bc36330 100644
--- a/Library/Homebrew/rubocops/formula_desc_cop.rb
+++ b/Library/Homebrew/rubocops/formula_desc_cop.rb
@@ -84,9 +84,8 @@ module RuboCop
           end
 
           # Check if a full stop is used at the end of a formula's desc
-          if regex_match_group(desc, /.*\.$/)
-            problem "Description shouldn't end with a full stop"
-          end
+          return unless regex_match_group(desc, /\.$/)
+          problem "Description shouldn't end with a full stop"
         end
 
         private
diff --git a/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb b/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb
index c82a1f2..aacb52e 100644
--- a/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb
+++ b/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb
@@ -96,9 +96,8 @@ describe RuboCop::Cop::FormulaAuditStrict::Desc do
         class Foo < Formula
           url 'http://example.com/foo-1.0.tgz'
           desc 'Description with a full stop at the end.'
-                                                       ^ Description shouldn\'t end with a full stop.
-          end
-      end
+                                                       ^ Description shouldn\'t end with a full stop
+        end
       RUBY
     end
 
@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Oct 28, 2017

Contributor

Me ending my Rubocop error message with a full stop was unfortunate given this PR. But is that the fix for the guard clause error, the return unless? Thank you so much!

Contributor

issyl0 commented Oct 28, 2017

Me ending my Rubocop error message with a full stop was unfortunate given this PR. But is that the fix for the guard clause error, the return unless? Thank you so much!

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 28, 2017

Contributor

Me ending my Rubocop error message with a full stop was unfortunate given this PR.

LOL. The same thing occurred to me :)

Yes the return unless is the guard clause. Unfortunately rubocop forces a different format for the last one in the function as opposed to all of the others. 🙈

Contributor

ilovezfs commented Oct 28, 2017

Me ending my Rubocop error message with a full stop was unfortunate given this PR.

LOL. The same thing occurred to me :)

Yes the return unless is the guard clause. Unfortunately rubocop forces a different format for the last one in the function as opposed to all of the others. 🙈

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Oct 28, 2017

Contributor

Ahhhh, that's why I had such trouble in the beginning with adding a new test and had to switch the old one around! It totally should have occurred to me. But thanks again. I just pushed the changes and fingers crossed it's green now! And now I know for next time!

Contributor

issyl0 commented Oct 28, 2017

Ahhhh, that's why I had such trouble in the beginning with adding a new test and had to switch the old one around! It totally should have occurred to me. But thanks again. I just pushed the changes and fingers crossed it's green now! And now I know for next time!

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Oct 29, 2017

Contributor

Yay, the build is green. Thanks very much for your help, everyone.

Contributor

issyl0 commented Oct 29, 2017

Yay, the build is green. Thanks very much for your help, everyone.

class Foo < Formula
url 'http://example.com/foo-1.0.tgz'
desc 'Description with a full stop at the end.'
^ Description shouldn\'t end with a full stop

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 29, 2017

Member

Are you sure you need to escape the \'? If so: no worries.

@MikeMcQuaid

MikeMcQuaid Oct 29, 2017

Member

Are you sure you need to escape the \'? If so: no worries.

This comment has been minimized.

@issyl0

issyl0 Oct 29, 2017

Contributor

The other occurrences of "shouldn't" in this file escape the apostrophe.

@issyl0

issyl0 Oct 29, 2017

Contributor

The other occurrences of "shouldn't" in this file escape the apostrophe.

# Check if a full stop is used at the end of a formula's desc
return unless regex_match_group(desc, /\.$/)
problem "Description shouldn't end with a full stop"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 29, 2017

Member

Could you add an autocorrect for this on line 105 below? Something like this would work: correction.gsub!(/\.$/, "")

@MikeMcQuaid

MikeMcQuaid Oct 29, 2017

Member

Could you add an autocorrect for this on line 105 below? Something like this would work: correction.gsub!(/\.$/, "")

formula_desc_cop: ensure no full stops at the end of desc
- This was a suggestion by Mike McQuaid in my `homebrew-core` audit
  description PR. Based on ilovezfs's incantation `grep -r -E 'desc
  ".*\."' *.rb`, some formulae descriptions do end in full stops. (My
  initial assessment of this failed to account for the fact that
  descriptions are strings and so end in `"`.)
- Add an autocorrect for this cop, too.

@MikeMcQuaid MikeMcQuaid merged commit 215f496 into Homebrew:master Oct 29, 2017

3 checks passed

codecov/patch 100% of diff hit (target 68.49%)
Details
codecov/project Absolute coverage decreased by -10.2% but relative coverage increased by +31.5% compared to 9dbff4e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 29, 2017

Member

Thanks again for your contribution to Homebrew, @issyl0! Without people like you submitting PRs we couldn't run this project. You rock!

Member

MikeMcQuaid commented Oct 29, 2017

Thanks again for your contribution to Homebrew, @issyl0! Without people like you submitting PRs we couldn't run this project. You rock!

@ilovezfs ilovezfs referenced this pull request Oct 29, 2017

Merged

Audit fixes for descriptions #20013

2 of 4 tasks complete
@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 29, 2017

Contributor

@issyl0 @MikeMcQuaid the autocorrect here doesn't actually work because the descriptions actually end in ' or " not in . so I think we need something like

-            correction.gsub!(/\.$/, "")
+            correction.gsub!(/\.(['"]?)$/, "\\1")

However that can lead to some undesirable behavior such as

-  desc "Encrypts your files, so you can safely store them in Dropbox, iCloud, ..."
+  desc "Encrypts your files, so you can safely store them in Dropbox, iCloud, "

I've replaced that description with

  desc "Encrypts your files, so you can safely store them in Dropbox, iCloud, etc."

on the assumption that we also need to white list descriptions ending in etc.

Contributor

ilovezfs commented Oct 29, 2017

@issyl0 @MikeMcQuaid the autocorrect here doesn't actually work because the descriptions actually end in ' or " not in . so I think we need something like

-            correction.gsub!(/\.$/, "")
+            correction.gsub!(/\.(['"]?)$/, "\\1")

However that can lead to some undesirable behavior such as

-  desc "Encrypts your files, so you can safely store them in Dropbox, iCloud, ..."
+  desc "Encrypts your files, so you can safely store them in Dropbox, iCloud, "

I've replaced that description with

  desc "Encrypts your files, so you can safely store them in Dropbox, iCloud, etc."

on the assumption that we also need to white list descriptions ending in etc.

@issyl0

This comment has been minimized.

Show comment
Hide comment
@issyl0

issyl0 Oct 29, 2017

Contributor

@ilovezfs That's true. I'll do a PR for whitelisting "etc", and also fix up the regex when I get a chance probably tomorrow. I also had a local branch for fixing the formulae affected by this new audit, but you beat me to it, so thanks!

Contributor

issyl0 commented Oct 29, 2017

@ilovezfs That's true. I'll do a PR for whitelisting "etc", and also fix up the regex when I get a chance probably tomorrow. I also had a local branch for fixing the formulae affected by this new audit, but you beat me to it, so thanks!

@issyl0 issyl0 deleted the issyl0:desc_no_full_stops branch Nov 1, 2017

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.