Skip to content

Fixes #103: Support parsing multiple exception types #244

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

Merged
merged 2 commits into from
May 26, 2018

Conversation

TysonAndre
Copy link
Contributor

Add a new array otherQualifiedNameList alongside qualifiedName
to CatchClause. (Contains remaining Tokens and QualifiedNames)
(This design breaks backwards compatibility as little as possible)

  • A future backwards incompatible release should merge the two
    properties into qualifiedNameList,
    or something along those lines.

Add a new helper method to parse the catch list.

  • The new helper is required because
    tests/cases/php-langspec/exception_handling/odds_and_ends.php
    should not fail because catch (int $e) is syntactically valid php
    (in 7.x), i.e. php --syntax-check doesn't reject it.

@TysonAndre
Copy link
Contributor Author

Could you publish a 0.0.12 release tag if this is merged?

This will help when using this library with PHP 7.1+ codebases.

Add a new array `otherQualifiedNameList` alongside qualifiedName
to `CatchClause`. (Contains remaining `Token`s and `QualifiedName`s)
(This design breaks backwards compatibility as little as possible)

- A future backwards incompatible release should merge the two
  properties into `qualifiedNameList`,
  or something along those lines.

Add a new helper method to parse the catch list.

- The new helper is required because
  `tests/cases/php-langspec/exception_handling/odds_and_ends.php`
  should not fail because `catch (int $e)` is **syntactically** valid php
  (in 7.x), i.e. `php --syntax-check` doesn't reject it.
@TysonAndre TysonAndre force-pushed the multi-class-catch branch from 7054436 to 60d3fc5 Compare May 13, 2018 02:26
@TysonAndre
Copy link
Contributor Author

Do the maintainers have any thoughts on this PR? E.g. documentation or additional tests that should be added?

If this is merged, #107 can be closed.

I think this should still become a list, just in a backwards incompatible change to 1.0 or 0.1 (e.g. the dependency https://github.com/felixfbecker/php-language-server/blob/master/composer.json#L28 of php-language-server would break if a name became a list of names)

A future backwards incompatible release should merge the two
properties into qualifiedNameList,
or something along those lines.

@roblourens
Copy link
Member

Sorry, I promise I'll get to your PRs ASAP

Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I agree that the CatchClause name qualifiedName should be a list, and this is the right solution to avoid breaking backcompat.

@roblourens roblourens merged commit 01479d7 into microsoft:master May 26, 2018
@TysonAndre TysonAndre deleted the multi-class-catch branch June 19, 2018 02:15
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