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

Add end location to all applicable elements #230

Merged
merged 10 commits into from
Dec 29, 2021

Conversation

arogachev
Copy link
Contributor

@arogachev arogachev commented Dec 28, 2021

@arogachev
Copy link
Contributor Author

Also could you help with setting up local test environment?

@arogachev
Copy link
Contributor Author

Also could you help with setting up local test environment?

Nevermind, I figured this out.

@jaapio
Copy link
Member

jaapio commented Dec 28, 2021

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. 🤔

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
Copy link
Member

jaapio commented Dec 28, 2021

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

# Conflicts:
#	src/phpDocumentor/Reflection/Php/Factory/Function_.php
#	src/phpDocumentor/Reflection/Php/Factory/Method.php
#	src/phpDocumentor/Reflection/Php/Function_.php
#	src/phpDocumentor/Reflection/Php/Method.php
@arogachev
Copy link
Contributor Author

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

Sure. Resolved merged conflicts.

@arogachev
Copy link
Contributor Author

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
Copy link
Member

jaapio commented Dec 29, 2021

Thanks for the update! this looks good.

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

@jaapio jaapio merged commit 91a7f79 into phpDocumentor:5.x Dec 29, 2021
@arogachev
Copy link
Contributor Author

arogachev commented Dec 29, 2021

Ah, you beat me with merging. 🙂 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 yiisoft/yii2-apidoc#263. 🎉

@arogachev arogachev deleted the 224-end-line branch December 29, 2021 10:38
@arogachev
Copy link
Contributor Author

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

@arogachev
Copy link
Contributor Author

Ah, you fixed it already. 👍 Newline was causing the error, got it.

@jaapio
Copy link
Member

jaapio commented Dec 29, 2021

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
Copy link
Contributor Author

OK. I'll try. 👌

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.

2 participants