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.

Sign up for GitHub

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

Jump to bottom

Resolve type aliases in docblocks #24

Merged
merged 1 commit into from
Jan 10, 2021
Merged

Resolve type aliases in docblocks #24

merged 1 commit into from
Jan 10, 2021

Conversation

CHItA
Copy link
Contributor

@CHItA CHItA commented Jan 6, 2021 •
edited
Loading

Issue: #23

@CHItA CHItA force-pushed the fix/resolve_aliased_names branch 2 times, most recently from 55294f4 to c25eeae Compare January 6, 2021 20:52
@CHItA
Copy link
Contributor Author

CHItA commented Jan 6, 2021

@williamdes the aliases from the context don't seem to be used elsewhere, so the later changes should work fine, and not break anyone's builds. I also added unit tests to prevent regression. I consider this PR finished and ready to be merged. If you find anything let me know.

williamdes
williamdes approved these changes Jan 7, 2021
View reviewed changes
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

This looks good, I will have another time a look onto your work

@williamdes williamdes added this to the v5.3.2 milestone Jan 7, 2021
@williamdes williamdes force-pushed the fix/resolve_aliased_names branch from c25eeae to 92e1d12 Compare January 7, 2021 19:20
@williamdes
Copy link
Member

Closing to try to trigger the CIs

@williamdes williamdes closed this Jan 7, 2021
@williamdes williamdes reopened this Jan 7, 2021
@williamdes williamdes force-pushed the fix/resolve_aliased_names branch 2 times, most recently from e097f6f to 050c039 Compare January 7, 2021 19:29
@CHItA
Copy link
Contributor Author

CHItA commented Jan 7, 2021

As far as I know

pull_request:
        branches:
            - main

should be enough, there is no need to match the name of my branch.

@williamdes
Copy link
Member

As far as I know

pull_request:
        branches:
            - main

should be enough, there is no need to match the name of my branch.

is that to specify the PR target ?

@CHItA
Copy link
Contributor Author

CHItA commented Jan 7, 2021

Yup.

@williamdes williamdes force-pushed the fix/resolve_aliased_names branch from 050c039 to b12d910 Compare January 7, 2021 19:36
@codecov
Copy link

codecov bot commented Jan 7, 2021 •
edited
Loading

Codecov Report

Merging #24 (1724f16) into main (76dd2c8) will increase coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #24      +/-   ##
============================================
+ Coverage     55.19%   55.43%   +0.23%     
  Complexity     1017     1017              
============================================
  Files            50       50              
  Lines          2607     2612       +5     
============================================
+ Hits           1439     1448       +9     
+ Misses         1168     1164       -4
Impacted Files Coverage Δ Complexity Δ
src/Parser/DocBlockParser.php 90.69% <100.00%> (+0.69%) 28.00 <7.00> (+1.00)
src/Parser/NodeVisitor.php 48.13% <100.00%> (+0.32%) 126.00 <0.00> (-1.00) ⬆️
src/Parser/ParserContext.php 63.33% <0.00%> (+6.66%) 26.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76dd2c8...1724f16. Read the comment docs.

@williamdes
Copy link
Member

Yup.

Awesome, thank you so much !

All tests pass except php 7.1
https://github.com/code-lts/doctum/pull/24/checks?check_run_id=1664642199#step:7:14

Would you too agree to drop support for PHP 7.1 ?

@CHItA
Copy link
Contributor Author

CHItA commented Jan 7, 2021

It is fine by me. Not sure why this would fail though.

@williamdes
Copy link
Member

It is fine by me. Not sure why this would fail though.

It fails because they have a version that is PHP <= 7.1 compatible and one that is compatible PHP > 7.1
Many hacks for some support..
Anyway, I will do more work on this before merging your work
Thank you for your help, this seems to be perfect

@williamdes williamdes force-pushed the fix/resolve_aliased_names branch from b12d910 to 53135a9 Compare January 7, 2021 20:12
@williamdes williamdes force-pushed the fix/resolve_aliased_names branch from 53135a9 to 1724f16 Compare January 7, 2021 20:14
@williamdes williamdes self-assigned this Jan 7, 2021
williamdes
williamdes approved these changes Jan 7, 2021
View reviewed changes
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

💯

williamdes added a commit that referenced this pull request Jan 10, 2021
@williamdes
Add a CHANGELOG entry for #24
5b33c47
@williamdes williamdes merged commit ef95618 into code-lts:main Jan 10, 2021
@CHItA CHItA deleted the fix/resolve_aliased_names branch January 10, 2021 22:40
@williamdes
Copy link
Member

I released 5.4.0-dev phar, you can try it out and let me know ;)

@CHItA
Copy link
Contributor Author

CHItA commented Jan 10, 2021

Seems to work great, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamdes williamdes williamdes approved these changes

Assignees

@williamdes williamdes

Labels
None yet
Milestone
v5.4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@CHItA @williamdes