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

utils: output what files edit is opening. #444

Merged
merged 1 commit into from
Jul 4, 2016
Merged

utils: output what files edit is opening. #444

merged 1 commit into from
Jul 4, 2016

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jul 3, 2016

  • 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 tests with your changes locally?

Since we've moved all formulae to taps it's not necessarily obvious what the path for the files are otherwise.

Since we've moved all formulae to taps it's not necessarily obvious
what the path for the files are otherwise.
@BrewTestBot BrewTestBot added the in progress Maintainers are working on this label Jul 3, 2016
@@ -431,6 +431,7 @@ def which_editor
end

def exec_editor(*args)
puts "Editing #{args.join "\n"}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't render very nicely if multiple arguments are supplied. May I suggest the following as a replacement?

ohai "Editing:", args

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it did render more nicely than that case as it's not truncated and the first argument (the typical case) is displayed on the same line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aesthetically, the changing indentation and that only the first line is prefixed with “Editing” for the atypical case is bothering me more, even if the common case becomes nicer and shorter.

But to be honest, I'll probably be slightly annoyed by this output anyway, no matter how it is styled, because I basically never want to know where the formula I'm editing is and when I do, my editor can tell me quickly and efficiently. But I'll certainly manage as I guess this is a minority opinion.

@UniqMartin
Copy link
Contributor

I guess the primary concern are formulae here, in which case something like brew which <formula> or something similar might be more appropriate? Once the file is open (I guess most often in a GUI editor), the path will be obvious from a context click on the editor's proxy icon (and the icon can be dragged to the terminal to get the full path into a command line).

On a side note, but not a serious suggestion for an alternative fix, this will also help:

HOMEBREW_EDITOR=echo brew edit qt5

@MikeMcQuaid
Copy link
Member Author

I guess the primary concern are formulae here, in which case something like brew which or something similar might be more appropriate? Once the file is open (I guess most often in a GUI editor), the path will be obvious from a context click on the editor's proxy icon (and the icon can be dragged to the terminal to get the full path into a command line).

I think this is pretty editor dependent. This is designed to improve the flow for first-time contributors where it might not be obvious where these files are. It's also useful for more regular contributors to have a file path to copy/paste to various other locations outside of the editor (particularly for those editors that don't have a proxy icon).

@UniqMartin
Copy link
Contributor

Yes, I understand the motivation for this PR and I imagine it will help the addressed user group. My needs and preferences are different, but that's not something that can (and should) stop this PR.

@MikeMcQuaid MikeMcQuaid merged commit 883b201 into Homebrew:master Jul 4, 2016
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Jul 4, 2016
@MikeMcQuaid
Copy link
Member Author

Cool, thanks @UniqMartin.

@MikeMcQuaid MikeMcQuaid deleted the edit-directory branch July 4, 2016 15:09
souvik1997 pushed a commit to souvik1997/brew that referenced this pull request Jul 25, 2016
Since we've moved all formulae to taps it's not necessarily obvious
what the path for the files are otherwise.
@Homebrew Homebrew locked and limited conversation to collaborators May 3, 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.

3 participants