phpDocumentor / Reflection

Reflection library to do Static Analysis for PHP Projects
MIT License
117 stars 51 forks source link

Add end location to all applicable elements #230

Closed arogachev closed 2 years ago

arogachev commented 2 years ago

A feature extracted from #224 / #225.

Use case - having links like this - https://github.com/yiisoft/yii2/blob/e459214a927424bafd7a23ed946951d4a74093cf/framework/db/ActiveRecord.php#L471-L474.

arogachev commented 2 years ago

Also could you help with setting up local test environment?

arogachev commented 2 years ago

Also could you help with setting up local test environment?

Nevermind, I figured this out.

jaapio commented 2 years ago

Thanks for this nice improvement! the unittest coverage is below the expected level, that's why the build is now failing.

If you have time to add the required tests that would be very nice, however seen the amount of work, and uncovered code I would accept a decreased coverage for now. :thinking:

Side note: I think we can improve the tests a bit by using an abstract testcase for our models so we can test the common methods, like getLocation and getEndLocation in a more flexible way.

jaapio commented 2 years ago

After merging the first PR, we to have some merge conflicts here. Could you please rebase your branch?

arogachev commented 2 years ago

After merging the first PR, we to have some merge conflicts here. Could you please rebase your branch?

Sure. Resolved merged conflicts.

arogachev commented 2 years ago

Again, thanks for a quick response.

I added more checks for getEndLocation(), this improved test coverage, but it's still slightly below the needed level. Maybe it has something to do with @covers / @inheritdoc tags, not really sure... Also added separate abstract test case for elements that you suggested.

What else you think needs to be tested? Or maybe it's acceptable for now?

jaapio commented 2 years ago

Thanks for the update! this looks good.

The coverage isn't much of an issue for now. We can always improve on this.

arogachev commented 2 years ago

Ah, you beat me with merging. :slightly_smiling_face: I added tests for previous PR and it resolved coverage problem. Then fixed almost all new CI errors. Trying to fix the last 2 codestyle errors now

Maybe you can help? Can't figure out this one yet.

We can fix this in a separate PR though.

Thanks! This was a last blocker for https://github.com/yiisoft/yii2-apidoc/issues/263. :tada:

arogachev commented 2 years ago

By the way could you make a new release with these changes? Would be nice instead of binding to commit in dependency.

arogachev commented 2 years ago

Ah, you fixed it already. :+1: Newline was causing the error, got it.

jaapio commented 2 years ago

I can create that release, later this week.

It would be nice to have the last part regarding the pretty printer in, so we have a nice new package of features :-). Do you have time to create a last pr for that one?

arogachev commented 2 years ago

OK. I'll try. :ok_hand: