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

Show cask description in brew cask info command #8335

Merged

Conversation

waldyrious
Copy link
Contributor

  • 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?
    (partially: I ran brew tests --online --only=cask/cmd/info, and all tests therein passed.)

Actually display Cask descriptions (introduced in #8137) in the output of brew cask info.

The approach I took here was to not display the description if it doesn't exist, instead of showing None as is done for the name(s). Happy to change it to show None if that's preferred (as descs are meant to eventually become mandatory). /cc @reitermarkus

@vitorgalvao
Copy link
Member

If we were planning on descriptions being optional, displaying nothing would be cleaner. But because we want them to be mandatory—and filled as soon as possible—I’d go with None because that will increase user awareness of the feature, meaning we might get more contributions.

@waldyrious waldyrious force-pushed the show-cask-descs-in-info-command branch from 84771d3 to 5393218 Compare August 13, 2020 21:24
@waldyrious
Copy link
Contributor Author

Ok, I added a commit adding the None output, and amended the previous commits to fix the failing tests that I hadn't noticed earlier.

@vitorgalvao
Copy link
Member

Fantastic and blazing fast work! Thank you for the continued contributions.

@waldyrious
Copy link
Contributor Author

Fantastic and blazing fast work!

I had actually implemented it this way at first, so the logic was still in RAM 😄

Thank you for the continued contributions.

My pleasure! I'm trying to get this desc stuff done while I have the time, before my vacations end at the end of this week :)

@reitermarkus reitermarkus merged commit 3b2a9c3 into Homebrew:master Aug 13, 2020
@reitermarkus
Copy link
Member

Thanks, @waldyrious!

@waldyrious waldyrious deleted the show-cask-descs-in-info-command branch August 13, 2020 22:58
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 17, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 17, 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.

None yet

4 participants