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.

Sign up for GitHub

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

Jump to bottom

formulary: handle @ formulae. #812

Merged
merged 3 commits into from
Aug 28, 2016
Merged

formulary: handle @ formulae. #812

merged 3 commits into from
Aug 28, 2016

Conversation

DomT4
Copy link
Member

@DomT4 DomT4 commented Aug 25, 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?

Useful for #620 & Homebrew/homebrew-core#971.

@BrewTestBot BrewTestBot added the in progress Maintainers are working on this label Aug 25, 2016
DomT4
DomT4 reviewed Aug 25, 2016
View reviewed changes
Library/Homebrew/cmd/search.rb
@@ -86,7 +86,7 @@ def search
metacharacters = %w[\\ | ( ) [ ] { } ^ $ * + ? .]
bad_regex = metacharacters.any? do |char|
ARGV.any? do |arg|
arg.include?(char) && !arg.start_with?("/")
arg.include?(char) && !arg.start_with?("/") && !arg.include?("@")
Copy link
Member Author

Choose a reason for hiding this comment

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

Open to opinions on whether this is sane. It might be a bit broad.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what the intent is here?

Copy link
Member

Choose a reason for hiding this comment

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

Ah because of the .? Could consider just removing that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it interprets the . in something like [email protected] as a failed desire to regex.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree that . without a modifier isn't a strong regex signal; the current version makes sense 👍

@DomT4 DomT4 added the discussion Input solicited from others label Aug 26, 2016
This was referenced Aug 26, 2016
Closed
Closed
MikeMcQuaid
MikeMcQuaid reviewed Aug 27, 2016
View reviewed changes
Library/Homebrew/formulary.rb
@@ -57,6 +57,7 @@ def self.class_s(name)
class_name = name.capitalize
class_name.gsub!(/[-_.\s]([a-zA-Z0-9])/) { $1.upcase }
class_name.tr!("+", "x")
class_name.gsub!(/\b[@]\b/, "AT")
Copy link
Member

Choose a reason for hiding this comment

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

Don't think you need the []. Also, maybe can go for /.@\d to enforce a digit (for now at least, we can tweak it later). After that 👍 to shipping this.

Copy link
Member Author

Choose a reason for hiding this comment

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

/.@\d matches l@1 in this case, which produces things like:

Expected to find class OpenssAT1, but only found: OpensslAT11.

I'm not sure how to shake that off in the context of gsub.

Copy link
Contributor

Choose a reason for hiding this comment

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

Capture parts of the matched text and use them in the replacement:

"[email protected]".gsub!(/(.)@(\d)/, "\\1AT\\2")

(And a sub! should be sufficient since we don't expect to have multiple @ in a formula name for now.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Pushed in the #832 PR.

@DomT4
formulary: translate @ to AT for classname
98155a5
@DomT4
test_formulary: add formula@version class translation check
24be5c4
@DomT4
search: handle [email protected] formulae.
ab203a7 
Before this change:
```
~> brew search [email protected]
[email protected] ✔
==> Did you mean to perform a regular expression search?
==> Surround your query with /slashes/ to search by regex.
```
@DomT4
Copy link
Member Author

DomT4 commented Aug 28, 2016

Will merge this as-is for now, with the redundant [] fixed. Feel free to tell me how I was being an idiot around the l@1 capture upon your return & I'll make any necessary changes 😄.

@DomT4 DomT4 merged commit 8e29cf1 into Homebrew:master Aug 28, 2016
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Aug 28, 2016
@DomT4 DomT4 deleted the at branch August 28, 2016 04:01
@DomT4 DomT4 mentioned this pull request Aug 28, 2016
Merged
4 tasks
@fm-techbts fm-techbts mentioned this pull request Aug 28, 2016
Closed
3 tasks
@apjanke
Copy link
Contributor

apjanke commented Aug 29, 2016

Use lookahead/lookbehind?

class_name.gsub!(/(?<=.)@(?=\d)/, "AT")

@DomT4
Copy link
Member Author

DomT4 commented Aug 29, 2016

Changed in #832 to (/(.)@(\d)/, "\\1AT\\2") per Martin's nudge. Don't have a strong preference either way, but if people prefer LA/LB I'm happy to change it again.

@MikeMcQuaid MikeMcQuaid mentioned this pull request Sep 4, 2016
Merged
5 tasks
@mistydemeo mistydemeo mentioned this pull request Apr 10, 2017
Merged
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
discussion Input solicited from others
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@DomT4 @apjanke @MikeMcQuaid @tdsmith @UniqMartin @BrewTestBot