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 MKVToolNix.app v9.9.0 #30351

Closed
wants to merge 2 commits into from
Closed

Add MKVToolNix.app v9.9.0 #30351

wants to merge 2 commits into from

Conversation

ponyspy
Copy link
Contributor

@ponyspy ponyspy commented Feb 24, 2017

If there’s a checkbox you can’t complete for any reason, that's okay, just explain in detail why you weren’t able to do so.

After making all changes to the cask:

  • brew cask audit --download {{cask_file}} is error-free.
  • brew cask style --fix {{cask_file}} reports no offenses.
  • The commit message includes the cask’s name and version.

Additionally, if adding a new cask:

  • Named the cask according to the token reference.
  • brew cask install {{cask_file}} worked successfully.
  • brew cask uninstall {{cask_file}} worked successfully.
  • Checked there are no open pull requests for the same cask.
  • Checked the cask was not already refused in closed issues.
  • Checked the cask is submitted to the correct repo.

@miccal
Copy link
Member

miccal commented Feb 24, 2017

How is this different from that found in Homebrew?

@miccal miccal added the awaiting user reply Issue needs response from a user. label Feb 24, 2017
homepage 'https://mkvtoolnix.download'

app "MKVToolNix-#{version}.app"
end
Copy link
Member

Choose a reason for hiding this comment

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

Missing a newline here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I do not understand what line is missing?
I found same cask and it's normal.

Copy link
Member

Choose a reason for hiding this comment

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

There should be an empty line after end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I fix it.

@ponyspy
Copy link
Contributor Author

ponyspy commented Feb 24, 2017

No deps, no build. Only binary app from authors web site.
It is easier and more convenient for the simple user.
Like me :)

@miccal miccal added awaiting maintainer feedback Issue needs response from a maintainer. and removed awaiting user reply Issue needs response from a user. labels Feb 24, 2017
@miccal
Copy link
Member

miccal commented Feb 25, 2017

@caskroom/maintainers:

audit for mkvtoolnix: warning
 - possible duplicate, cask token conflicts with Homebrew core formula:
https://github.com/Homebrew/homebrew-core/blob/master/Formula/mkvtoolnix.rb

@vitorgalvao
Copy link
Member

I don’t have a strong opinion on this one, but perhaps we should refuse it.

We have to draw the line somewhere, and the deduplication effort happened for a reason. We were aware some apps might be a bit more of an inconvenience to use, but that deduplication was more important. I don’t see much request for this one (we used to have it). I’m betting it’ll become abandoned pretty quickly.

@claui
Copy link
Contributor

claui commented Feb 25, 2017

Accepting the cask would make brew search mkvtoolnix yield:

mkvtoolnix
Caskroom/cask/mkvtoolnix

At that point, the user will have to make a choice; however, either choice would lead to the same result.

In RPGs, this is considered an annoyance. In all other places, I consider it poor UX.

@vitorgalvao
Copy link
Member

Good thing it’s already pretty late and I’m going to bed now. Otherwise I’d lose the rest of my day on tvtropes!

But I know I’ll check it tomorrow. I wonder if I’ll be able to restrain myself to just that page.

@miccal
Copy link
Member

miccal commented Feb 27, 2017

@ponyspy in the spirit of deduplication, we are rejecting this Cask.

Thank you for your contribution, though.

@miccal miccal closed this Feb 27, 2017
@miccal miccal removed the awaiting maintainer feedback Issue needs response from a maintainer. label Feb 27, 2017
@ponyspy
Copy link
Contributor Author

ponyspy commented Feb 27, 2017

@miccal What about this cask or this? Remove too? I use this programs and i install it from cask, not brew.

I don't understand where you see a duplicate... I want to program without dependencies, and as fast as possible, I thought the philosophy of cask in it.

Very strange...

@claui
Copy link
Contributor

claui commented Feb 27, 2017

@ponyspy I see your point, and how decisions like the one at hand are prone to come across as irritating, or seemingly arbitrary. Please be aware that no matter what, contributions like yours are appreciated a lot. Even if a cask may ultimately end up being rejected, we are thankful for people like you who bring such edge cases to our attention.

Regarding mpv:

The duplication was already investigated several times ([1] [2]); it has been found that they are not the same thing so it’s completely justified to keep both around.

Regarding djview:

I’m with you here; the Homebrew formula is redundant and should be deleted (it is broken anyway, at least on my machine). @vitorgalvao has in fact brought this to Homebrew’s attention some time ago; the thing is that the housework that is necessary before we delete those duplicates is not trivial at all and is therefore still in progress.

I admit that in theory, we could make such decisions transparent to users at all times. In practice, however, I believe this is incredibly hard; the available resources and energy are certainly better spent on pushing the project forward.

No matter what, I’m looking forward to your future contributions. 👍

@Homebrew Homebrew locked and limited conversation to collaborators May 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants