Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

p4v 18.3-1719707: Add CLI-accessible commands to work with Git out of the box #54957

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

pete-woods
Copy link
Contributor

@pete-woods pete-woods commented Nov 14, 2018

  • Wrapper script needed so that its embedded Qt can be found
    (i.e. just symlinking won't work, similar to kdiff3)

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.
  • The submission is for a stable version or documented exception.

@pete-woods
Copy link
Contributor Author

Does anyone have time to have a look over this pull request? It should be pretty straightforward.

@vitorgalvao
Copy link
Contributor

Thank you, but we don’t make CLI commands out of Contents/MacOS unless it’s specifically recommended upstream (is it?).

@pete-woods
Copy link
Contributor Author

Well the "proper" script in resources for p4merge just segfaults.

@pete-woods
Copy link
Contributor Author

@pete-woods
Copy link
Contributor Author

Not too helpful to just immediately close this pull request, when there are other things I can try.

@pete-woods
Copy link
Contributor Author

I'm going to see if I can make the proper script not segfault.

@pete-woods
Copy link
Contributor Author

I got the inspiration for this from https://github.com/Homebrew/homebrew-cask/blob/master/Casks/kdiff3.rb which does the same thing linking into Contents/MacOS, btw.

@pete-woods
Copy link
Contributor Author

Though that, I guess importantly, includes a reference to #18809

@pete-woods
Copy link
Contributor Author

Just reading the PR for that change, to see what the discussion was: #35967

@pete-woods
Copy link
Contributor Author

Actually, I just read that p4 docs page fully, and found this quote:

An alternative to using shell script is to add an "alias" to the shell profile. For example, for the bash shell add the following line to the .bashrc file in the home directory:

alias p4merge='/Applications/p4merge.app/Contents/MacOS/p4merge

@pete-woods
Copy link
Contributor Author

Does that make the shim script legit?

@vitorgalvao
Copy link
Contributor

Not too helpful to just immediately close this pull request, when there are other things I can try.

I’m tired of typing this, but closing an issue does not mean the conversation is over, as evidenced by the fact you’re still able to make that comment. Closing does not mean locking or deleting, it means archiving.

In fact, I specifically invited you to reply when I asked (emphasis added):

unless it’s specifically recommended upstream (is it?).

The most common response when “there are other things to try” and we leave the issue open is silence. Nothing at all. It’s another lingering issue left in the way of trying to tackle them all. Closing is way more efficient. Had you said nothing, time saved. Since you did, no harm done. I can even reopen the issue!

I got the inspiration for this from https://github.com/Homebrew/homebrew-cask/blob/master/Casks/kdiff3.rb which does the same thing linking into Contents/MacOS, btw.

The majority of the times I point out a rule and someone else points out “but this other cask does it” ends in either “because that cask has this particularity” or “and that one is also wrong”.

Does that make the shim script legit?

Yes, but please edit and follow the same pattern as the others (adding the empty line and comment).

Finally, please don’t post so much in a row. This is a not a chat app, and it makes the issue more difficult to parse and fills my inbox.

@vitorgalvao vitorgalvao reopened this Nov 18, 2018
@vitorgalvao vitorgalvao added the awaiting user reply Issue needs response from a user. label Nov 18, 2018
… the box

- Wrapper script needed so that its embedded Qt can be found
  (i.e. just symlinking won't work, similar to kdiff3)
@pete-woods
Copy link
Contributor Author

Apologies for directionless complaining, it's not the most productive thing to do.

Firstly, I've tried to update the patch as you requested, though the style checker doesn't want me to put a blank line in, so I'm probably misinterpreting where you want it.

With regards to my earlier griping, I would only say that it's pretty disheartening to see the first thing in response to a patch you've worked on being "patch closed". I now understand that's your workflow for this repo, and will update my expectations accordingly.

I would, however, suggest that a bot to auto close stale tickets for you (even with a relatively short timeout) is a lot more welcoming, especially to first time contributors.

@vitorgalvao
Copy link
Contributor

Apologies for directionless complaining, it's not the most productive thing to do.

Thank you for the words.

I would, however, suggest that a bot to auto close stale tickets for you

Noted. We do have one, but it only works for issues (i.e. not PRs).

@vitorgalvao vitorgalvao merged commit 12a2566 into Homebrew:master Nov 19, 2018
@pete-woods
Copy link
Contributor Author

Thanks for the merge! 👍

@lock lock bot locked and limited conversation to collaborators Dec 20, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
awaiting user reply Issue needs response from a user.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants