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

Combine sbarex-qlmarkdown with qlmarkdown for compatibility #114374

Closed
wants to merge 2 commits into from

Conversation

elsiehupp
Copy link

Important: Do not tick a checkbox if you haven’t performed its action. Honesty is indispensable for a smooth review process.

In the following questions <cask> is the token of the cask you're submitting.

After making all changes to a cask, verify:

Additionally, if adding a new cask:

  • Named the cask according to the token reference.
  • Checked the cask was not already refused.
  • Checked the cask is submitted to the correct repo.
  • brew audit --new-cask <cask> worked successfully.
  • brew install --cask <cask> worked successfully.
  • brew uninstall --cask <cask> worked successfully.

I'm not sure if this is the correct approach, but the problem I'm trying to solve is that the existing qlmarkdown is abandoned and does not work on Catalina or later (and this is a known issue), while sbarex-qlmarkdown works exclusively with Catalina or later but has limited discoverability.

I haven't consulted with either @toland or @sbarex about this, and the README at sbarex/QLMarkdown would need to be updated with the new cask name.

I did install and uninstall on both macOS Big Sur and macOS High Sierra, but I'm not sure if I ran them correctly. brew install --cask path/to/qlmarkdown.rb on Big Sur reported no errors correctly and correctly installed the application, while on High Sierra it reported no errors but seemingly did not actually install anything. In both cases brew uninstall --cask path/to/qlmarkdown.rb worked correctly.

Signed-off-by: Elsie Hupp <9206310+elsiehupp@users.noreply.github.com>
@miccal
Copy link
Member

miccal commented Nov 17, 2021

Thank you @elsiehupp but this is not the way to handle this.

I will change the name of the old qlmarkdown Cask to toland-qlmarkdown and mark it as discontinued, and then rename the old sbarex-qlmarkdown Cask to just qlmarkdown.

@elsiehupp
Copy link
Author

@miccal yes, I wasn’t sure if this was the correct approach, so thanks for fixing the underlying problem regardless!

FWIW you probably do want something like depends_on macos: "< :catalina" on toland-qlmarkdown and something like depends_on macos: ">= :catalina" on sbarex-qlmarkdown (now just qlmarkdown). (It’s unclear whether or not toland-qlmarkdown runs on Catalina.)

@miccal
Copy link
Member

miccal commented Nov 17, 2021

@miccal yes, I wasn’t sure if this was the correct approach, so thanks for fixing the underlying problem regardless!

No problem.

FWIW you probably do want something like depends_on macos: "< :catalina" on toland-qlmarkdown

This seems to work on Catalina and higher if installed with the --no-quarantine flag, so unless those requirements are specified upstream we would not add it.

and something like depends_on macos: ">= :catalina" on sbarex-qlmarkdown (now just qlmarkdown). (It’s unclear whether or not toland-qlmarkdown runs on Catalina.)

This one already has a depends_on macos: ">= :catalina".

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants