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 a desc stanza to the cask DSL #8137

Merged

Conversation

waldyrious
Copy link
Contributor

@waldyrious waldyrious commented Jul 29, 2020

Fixes Homebrew/homebrew-cask#77696.

This PR introduces a new desc stanza to the Homebrew Cask DSL, to allow providing a proper description of a package, since the name may not always be sufficiently informative. This is similar to the desc field of Homebrew formulas, introduced by @adzenith in 8636547 (Homebrew/legacy-homebrew#39911), back in May 2015.

For example, for the fontforge.rb cask, the contents would become something like this:

   name 'FontForge'
+  desc 'Font editor and converter for outline and bitmap fonts'
   homepage 'https://fontforge.github.io/en-US/'

Note: I have very little experience with Ruby and Homebrew's internals, so this change was done mostly by studying the existing code (following an initial pointer by @reitermarkus) for patterns in similar stanzas (especially the name one), as well as previous PRs such as @vladimyr's #7286. I listed the changes inferred from that study in this comment, which was confirmed by @reitermarkus.

After this PR, I will make the necessary adjustments to the documentation in the Homebrew/homebrew-cask repo.

  • 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 style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Note: I've ran brew tests locally "successfully", in that all the failures were timeout ones apparently unrelated to these changes.

EDIT: for future reference (e.g. for someone reading this to figure out how one should go about adding a new stanza), the core of this PR is commit ffed8fd, which implements the actual desc stanza, along with 3b86e1f, which implements the rubocop to check a cask's desc the same was that is done for formula descs.

@waldyrious waldyrious changed the title Fix URL of the documentation for stanza order and grouping Add a desc stanza to the cask DSL Jul 29, 2020
@MikeMcQuaid MikeMcQuaid added the cask Homebrew Cask label Jul 29, 2020
@MikeMcQuaid MikeMcQuaid requested a review from a team July 29, 2020 15:47
@core-code
Copy link
Contributor

i can 'donate' a few thousand app-descriptions. quality is varying but it could be used a starting point.

Library/Homebrew/cask/cmd/create.rb Outdated Show resolved Hide resolved
@reitermarkus
Copy link
Member

We'll also need to add the same audit which already exists for formulae to Cask::Audit.

@waldyrious
Copy link
Contributor Author

We'll also need to add the same audit which already exists for formulae to Cask::Audit.

I couldn't locate the existing audit for formulae; can you point me to it?

@reitermarkus
Copy link
Member

reitermarkus commented Jul 30, 2020

can you point me to it?

Ah, it's actually a RuboCop rule: rubocops/formula_desc.rb

Probably the best way to go is:

  1. Move validation code into a new module which can be shared.
  2. Add new RuboCop rule for casks in rubocops/cask. As a reference, rubocops/cask/homepage_url_trailing_slash.rb is probably the simplest rule to have a look at.

@waldyrious
Copy link
Contributor Author

Thanks for the pointers, but I must confess I'm a little out of my depth here, as I have pretty much zero experience programming in Ruby. I have several basic questions, such as:

  • Where would this module live in the file tree, and what would it be named?
  • What would it look like in terms of structure and content? I don't suppose I can simply copy the audit_formula code from formula_desc.rb into a new file, so there'd have to be some refactoring.
  • How would I use it in the new rule (say, rubocops/cask/desc_valid.rb) and in the existing formula audit at rubocops/formula_desc.rb? I mean, what's the syntax for doing so — extend, include, something else?

Do we have an example of a similar structure I can study? Or otherwise, would you mind drafting a pseudo-code outline of the suggested changes?

(I imagine these may be simple changes for someone experienced, so feel free to add a commit to this PR with the intended changes if that's more convenient to you.)

@reitermarkus
Copy link
Member

reitermarkus commented Jul 30, 2020

I'd start by creating a rubocops/shared/desc_helper.rb with a helper module and function, i.e.

module RuboCop
  module Cop
    module DescHelper
      module_function

      def audit_desc(type, name, desc)
      end
    end
  end
end

which returns an array of problems.

Called like DescHelper.audit_desc(:formula, formula_name, desc).

@waldyrious
Copy link
Contributor Author

waldyrious commented Jul 30, 2020

@reitermarkus I'm sorry but I really lack the background to implement what you're suggesting. I have been making experiments locally for the past couple hours, and testing with brew tests, but I haven't been able to make much progress, and am now blocked with a NoMethodError: undefined method `string_content' which I can't seem to get rid of.

I'll add a WIP commit to this PR to show you what I've got so far. I'd appreciate some more specific feedback/directions/pointers, as I really have no experience with Ruby programming so I lack many of the common patterns or concepts that would allow me to advance in this more autonomously.

Copy link
Member

@reitermarkus reitermarkus left a comment

Choose a reason for hiding this comment

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

Step 1 should work after these changes.

Library/Homebrew/rubocops/shared/desc_helper.rb Outdated Show resolved Hide resolved
Library/Homebrew/rubocops/shared/string_content.rb Outdated Show resolved Hide resolved
Library/Homebrew/rubocops/shared/desc_helper.rb Outdated Show resolved Hide resolved
Library/Homebrew/rubocops/shared/desc_helper.rb Outdated Show resolved Hide resolved
Library/Homebrew/rubocops/shared/desc_helper.rb Outdated Show resolved Hide resolved
@waldyrious
Copy link
Contributor Author

Thanks for the detailed feedback, @reitermarkus! I've made the changes you suggested, and through trial and error, continued to extract more functions from rubocops/extend/formula.rb into separate modules to be used in rubocops/shared/desc_helper.rb, in an attempt to get the tests passing again. Since the number of extracted modules kept growing, I decided to combine the newer ones in a single module for the moment, which I provisionally called rubocops/shared/helper_functions.rb.

I am however stuck with the following errors:

NoMethodError:
        undefined method `add_offense' for RuboCop::Cop::DescHelper:Module

and

NameError:
        uninitialized constant RuboCop::Cop::FormulaAudit::Desc::VALID_LOWERCASE_WORDS

I've tried, among other things, to locate definitions of add_offense elsewhere in the project and add them to helper_functions.rb, and copied VALID_LOWERCASE_WORDS to desc_helper.rb, but without success; so I thought I'd submit the updated code for you to take a look at; please let me know how to proceed.

@reitermarkus
Copy link
Member

reitermarkus commented Aug 1, 2020

The helper function should return an array of problems instead of calling the problem method. This way you can leave the problem method where it is now and just loop through the returned problems.

@waldyrious
Copy link
Contributor Author

(Apologies, I forgot to add the newly created files. Updated the commit now.)

@waldyrious
Copy link
Contributor Author

The helper function should return an array of problems instead of calling the problem method. This way you can leave the problem method where it is now and just loop through the returned problems.

Ok, thanks — I'll try to implement this and will provide an update.

@waldyrious
Copy link
Contributor Author

waldyrious commented Aug 1, 2020

Hi @reitermarkus — I've added a new commit with additional changes. The files should now pass brew style so you will be able to see the failures in the GitHub actions check as I do locally. The issue now seems to be the formatting of the rubocob offense. I'm not sure how to adjust the code to make it match the previous behavior. For example, here's one of the failures I'm getting: in RuboCop::Cop::FormulaAudit::Desc When auditing formula desc When wrong "command-line" usage in desc, the expected/got diff is:

@@ -1,7 +1,6 @@
 class Foo < Formula
   url 'https://brew.sh/foo-1.0.tgz'
   desc 'command line'
-        ^ Description should start with a capital letter
-        ^^^^^^^^^^^^ Description should use "command-line" instead of "command line"
+  ^^^^^^^^^^^^^^^^^^^ Description should use "command-line" instead of "command line"
 end

The other failures all follow the same pattern.

@reitermarkus reitermarkus force-pushed the add-desc-stanza-to-cask-dsl branch 2 times, most recently from e49ebe0 to ae653c3 Compare August 7, 2020 21:35
@reitermarkus reitermarkus requested a review from a team August 7, 2020 22:59
Copy link
Contributor Author

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

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

Thanks for doing the rubocop implementation, @reitermarkus! I left some comments inline (mostly nitpicks); please take a look and let me know what you think. I'd be happy to make the necessary changes myself, of course.

Library/Homebrew/rubocops/cask/desc.rb Outdated Show resolved Hide resolved
Library/Homebrew/rubocops/shared/desc_helper.rb Outdated Show resolved Hide resolved
Library/Homebrew/rubocops/shared/desc_helper.rb Outdated Show resolved Hide resolved
Library/Homebrew/rubocops/shared/desc_helper.rb Outdated Show resolved Hide resolved
Library/Homebrew/rubocops/shared/desc_helper.rb Outdated Show resolved Hide resolved
Library/Homebrew/rubocops/shared/string_content.rb Outdated Show resolved Hide resolved
@reitermarkus
Copy link
Member

reitermarkus commented Aug 8, 2020

@waldyrious, yes, please feel free to make these changes yourself.

Also, we need to add the cop to .rubocop_ ask.yml as well.

@waldyrious
Copy link
Contributor Author

waldyrious commented Aug 8, 2020

@waldyrious, yes, please feel free to make these changes yourself.

Sure, will do! Please do confirm the correct course of action on the comments I left inline, though, as I'm not sure which decision is the preferred in some of them.

@waldyrious
Copy link
Contributor Author

Hey @reitermarkus, I've done the discussed changes, and cleaned up the commits to make them more atomic. Let me know if there are any additional changes left.

@waldyrious
Copy link
Contributor Author

waldyrious commented Aug 8, 2020

By the way, for future/self reference, I had some trouble figuring out the instructions from brew tests's manpage, specifically how to run a subset of the tests:

--only: Run only <test_script>_spec.rb. Appending :<line_number> will start at a specific line.

In practice this means that for a test file like Library/Homebrew/test/rubocops/cask/desc_spec.rb, the command should be:

$ brew tests --only=rubocops/cask/desc

(Hopefully this will help someone else —or my future self— who gets confused by the manpage instructions.)

@reitermarkus reitermarkus merged commit d5dfe4d into Homebrew:master Aug 8, 2020
@reitermarkus
Copy link
Member

Thanks, @waldyrious, nice work!

@waldyrious waldyrious deleted the add-desc-stanza-to-cask-dsl branch August 9, 2020 08:47
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 18, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cask Homebrew Cask outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: add desc stanza
5 participants