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 exception message of ExtractionException #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pierallard
Copy link
Contributor

When there was an issue on ClassNameExtractor or NamespaceExtractor, the Exception does not link to any file.
This PR improve the exception message to include the errored file.

@@ -28,8 +28,8 @@ public function parse(\SplFileInfo $file)

$content = file_get_contents($file->getRealPath());
$tokens = Tokens::fromCode($content);
$classNamespace = $namespaceExtractor->extract($tokens);
$className = $classNameExtractor->extract($tokens);
$classNamespace = $namespaceExtractor->extract($tokens, $file);
Copy link
Collaborator

Choose a reason for hiding this comment

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

i would catch the ExtractionException here and throw a new ParserException indicating the failing file path.

@pierallard pierallard force-pushed the improve-error-message branch 2 times, most recently from 3eabc88 to 225459c Compare June 20, 2017 16:06
/**
* @param \SplFileInfo $file
*
* @throws ParseException
Copy link
Collaborator

Choose a reason for hiding this comment

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

ParseException or ParsingException ?

/**
* Class ParseException.
*
* @author Pierre Allard <pierre.alalrd@akeneo.com>
Copy link
Collaborator

Choose a reason for hiding this comment

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

allard

@pierallard pierallard force-pushed the improve-error-message branch 4 times, most recently from af45442 to 392d04f Compare June 20, 2017 16:15
Copy link
Contributor

@jjanvier jjanvier left a comment

Choose a reason for hiding this comment

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

I don't understand why you need to create a new exception for that, but ok

@pierallard
Copy link
Contributor Author

@jjanvier Because @nidup ask me to do this in his review :)

@nidup
Copy link
Collaborator

nidup commented Jun 20, 2017

To avoid to pass the file in the extractor to throw a more accurate exception :)

$className = $classNameExtractor->extract($tokens);
try {
$classNamespace = $namespaceExtractor->extract($tokens, $file);
$className = $classNameExtractor->extract($tokens, $file);
Copy link
Collaborator

@nidup nidup Jun 23, 2017

Choose a reason for hiding this comment

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

you pass a useless $file param here! (on both lines)

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.

None yet

3 participants