Skip to content

Conversation

vigdorchik
Copy link
Contributor

...cTrees

for given fragments. Don't type-check when forcing doc comments, but rather
do it directly. Test the new functionality as well as better tests for
the old one.

@vigdorchik
Copy link
Contributor Author

Review by @huitseeker.

… docTrees

for given fragments. Don't type-check when forcing doc comments, but rather
 do it directly. Test the new functionality as well as better tests for
the old one.
@ghost ghost assigned scala-jenkins Feb 26, 2013
@JamesIry
Copy link
Contributor

ping @huitseeker

@vigdorchik
Copy link
Contributor Author

Review by @dragos

@huitseeker
Copy link
Contributor

I'm very sorry for the time it took me to get back to this review. This is a great PR, which deserved swifter treatment (I'm marred by build problems I'm only solving now — again, I'm sorry). An unrelated test fails reliably (@dragos reproduced the problem : test/files/run/reflection-java-annotations), on this PR as well as its merge-base — it shouldn't hinder the merge in any way.

LGTM !

@xeno-by
Copy link
Contributor

xeno-by commented Mar 1, 2013

If you rebase against the head of 2.10.x, the test failure will be fixed.

@vigdorchik
Copy link
Contributor Author

@JamesIry ping. Should I rebase?

@JamesIry
Copy link
Contributor

JamesIry commented Mar 5, 2013

Let's find out...

@JamesIry
Copy link
Contributor

JamesIry commented Mar 5, 2013

PLS REBUILD ALL

@scala-jenkins
Copy link

(kitty-note-to-self: ignore 14447871)
🐱 Roger! Rebuilding pr-rangepos-per-commit, pr-checkin-per-commit for 53c499b. 🚨

@adriaanm
Copy link
Contributor

adriaanm commented Mar 5, 2013

it builds ok: https://scala-webapps.epfl.ch/jenkins/job/pr-checkin-per-commit/856/console

merging so @paulp 's refactoring isn't blocked

adriaanm added a commit that referenced this pull request Mar 5, 2013
SI-7109 SI-7153 Generalize the API to get docComments: allow to force do...
@adriaanm adriaanm merged commit 93c2a5b into scala:2.10.x Mar 5, 2013
@paulp
Copy link
Contributor

paulp commented Mar 5, 2013

@adriaanm but of course I really need it to make it all the way to master. I can merge 2.10.x into master if necessary, unless someone is already?

@adriaanm
Copy link
Contributor

adriaanm commented Mar 5, 2013

I just submitted a PR.

On Tue, Mar 5, 2013 at 11:18 AM, Paul Phillips notifications@github.comwrote:

@adriaanm https://github.com/adriaanm but of course I really need it to
make it all the way to master. I can merge 2.10.x into master if necessary,
unless someone is already?


Reply to this email directly or view it on GitHubhttps://github.com//pull/2156#issuecomment-14459316
.

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.

7 participants