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

Improve coverage #24 #32

Merged
merged 14 commits into from
Jul 7, 2015
Merged

Improve coverage #24 #32

merged 14 commits into from
Jul 7, 2015

Conversation

asgrim
Copy link
Member

@asgrim asgrim commented Jul 3, 2015

Link to issue #24

@asgrim asgrim self-assigned this Jul 3, 2015
@asgrim asgrim added this to the 1.0.0 milestone Jul 3, 2015
@asgrim asgrim mentioned this pull request Jul 3, 2015
6 tasks
@asgrim asgrim force-pushed the improve-coverage-issue-24 branch from f4fcbf8 to 7cbe796 Compare July 3, 2015 13:04
@asgrim asgrim changed the title [WIP] Improve coverage #24 Improve coverage #24 Jul 5, 2015
@asgrim asgrim assigned Ocramius and unassigned asgrim Jul 5, 2015
@asgrim
Copy link
Member Author

asgrim commented Jul 5, 2015

Note to @Ocramius: coverage is not complete with this PR, it will just help if we get progress so far merged in. We'll leave #24 open to continue to track coverage improvements

@@ -46,11 +46,21 @@ public function getName()
*/
public function getDisplayName()
{
return ucfirst(basename($this->name));
return basename(str_replace('\\', '/', $this->name));
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

because it didn't work before, writing the test discovered it did nothing ;)

Copy link
Member

Choose a reason for hiding this comment

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

So what is being done here? Is it just a user-friendly identifier?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, exactly, it turns \BetterReflection\Reflection\ReflectionClass into just ReflectionClass for example

Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename the method to make that clear?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed this method, probably not required anyway

Ocramius added a commit that referenced this pull request Jul 7, 2015
@Ocramius Ocramius merged commit 0205ce5 into master Jul 7, 2015
@Ocramius Ocramius deleted the improve-coverage-issue-24 branch July 7, 2015 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants