Skip to content

Fix for issue 294 (definitions search displays comment lines) #681

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

Merged
merged 3 commits into from
Dec 9, 2013

Conversation

acdevlin
Copy link
Contributor

@acdevlin acdevlin commented Nov 1, 2013

Issue 294: #294

Example screenshots of this change in action: http://imgur.com/a/nuwxE#0

I signed and returned a personal Oracle Contributor Agreement, but have not yet heard a response back. Unsure of what the turnaround time on those requests is normally like. Here is the pull request in preparation.

@ghost ghost assigned kahatlen Nov 4, 2013
@vladak
Copy link
Member

vladak commented Nov 4, 2013

Displaying just the defs when doing defs search makes sense to me. I like the way how you presented the explanation, very concise. BTW you can upload the pictures to this site as well (drag-n-drop).

@acdevlin
Copy link
Contributor Author

acdevlin commented Nov 7, 2013

Ah, will remember that for next time. And thanks for the kind words :)

@@ -202,8 +202,9 @@ public static void prettyPrint(Writer out, SearchHelper sh, int start,
FileReader r = genre == Genre.PLAIN
? new FileReader(new File(sh.sourceRoot, rpath))
: null;
boolean isDefSearch = (sh.builder.getDefs() != null) ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is basically the only problem I have with this patch
suppose I want to search for "foo and defs:bar"
with above in place it will just show me bar ...
so I'd relax this to IF ONLY def field is filled in ...

still it won't be a proper solution, but until we have lucene highlighter in place ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point; we very rarely use more elaborate searches like this, so for our own personal case this issue has never come up.

Will restrict it to ONLY defs field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After much delay due to work busy-ness, fixed in latest commit.

Fair warning: The logic to check for only the def field being populated currently lives in QueryBuilder.java since it made more sense to me. Can easily be moved/reformatted/etc if anyone has concerns with that.

@ghost ghost assigned tarzanek Dec 6, 2013
@tarzanek
Copy link
Contributor

tarzanek commented Dec 6, 2013

OK, looks fine to me
just the usual OCA questions - I need you to either send me your OCA # (get it by following http://www.oracle.com/technetwork/community/oca-486395.html )
or agree with OCA here - so write you agree with OCA here(for this patch)
so I am able to accept this patch from legal point of view

@acdevlin
Copy link
Contributor Author

acdevlin commented Dec 6, 2013

I had to re-submit my OCA a few days ago, so it's still being processed.

In the interest of getting this pushed in more quickly: I hereby agree with the Oracle Contributor Agreement (OCA) on an individual basis for this patch. Just so we're all legally squared away 👍

tarzanek added a commit that referenced this pull request Dec 9, 2013
Fix for issue 294 (definitions search displays comment lines)
@tarzanek tarzanek merged commit a907c98 into oracle:master Dec 9, 2013
@tarzanek
Copy link
Contributor

tarzanek commented Dec 9, 2013

Thank you sir! ;)

@tarzanek
Copy link
Contributor

;) and now I see you didn't compile tests ...
oh well, will fix ...

tarzanek pushed a commit that referenced this pull request Dec 11, 2013
tarzanek pushed a commit that referenced this pull request Dec 11, 2013
@acdevlin
Copy link
Contributor Author

Whoops, I knew I was forgetting something...my mistake. Sorry!

@tarzanek
Copy link
Contributor

well you were not the only one, looking at the code I wrote ... I could have saved one if ... will rewrite ;)

tarzanek pushed a commit that referenced this pull request Dec 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants