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

Fix full class names #215

Merged
merged 14 commits into from
Feb 11, 2022
Merged

Fix full class names #215

merged 14 commits into from
Feb 11, 2022

Conversation

tianyiw2013
Copy link
Contributor

@tianyiw2013 tianyiw2013 commented Dec 10, 2021

Change Summary:

Checks:

  • CHANGELOG.md updated with relavent changes

@tianyiw2013
Copy link
Contributor Author

Test Suite Failing
I don't know how to solve the problem. Help me @neild3r
https://github.com/neild3r/vscode-php-docblocker/runs/4485797651?check_suite_focus=true

@tianyiw2013 tianyiw2013 changed the title fix full class names (#214) fix full class names Dec 10, 2021
@neild3r
Copy link
Owner

neild3r commented Dec 10, 2021

Hmmm I'm not too sure I've had this before but re-running has always helped. This time it doesn't seem to be doing the same trick

@tianyiw2013
Copy link
Contributor Author

lcov.info no content generated.
I studied it for a long time, but it still didn't work properly.

@tianyiw2013
Copy link
Contributor Author

Submitted again and again, and finally passed.

@neild3r
Copy link
Owner

neild3r commented Dec 13, 2021

We definitely need to work out why it's being so inconsistent as this is frustrating. Perhaps it just needs a delay in the git hub action or something.

Copy link
Owner

@neild3r neild3r left a comment

Choose a reason for hiding this comment

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

Hi @tianyiw2013,

Sorry for the delay in getting back to you on this. Happy new year and thanks for you work to date. I have a few bits of feedback.

  • Would it be possible to break up the getFullyQualifiedType method into more methods.
  • There should probably be a getClassesFromUse method or something similar which just works with an individual line.
  • Please could we also use camelCase instead of snake_case

If you do not understand please let me know I understand english isn't your first language.

@tianyiw2013 tianyiw2013 requested a review from neild3r January 4, 2022 03:38
@tianyiw2013
Copy link
Contributor Author

Thank you for your feedback. Is this OK now?

Test Suite failed again. -_-

@neild3r
Copy link
Owner

neild3r commented Jan 7, 2022

@tianyiw2013 yes this is looking much neater thank you. I will look at the test suite issue as this is not on you for whatever reason it's being inconsistent

@neild3r
Copy link
Owner

neild3r commented Feb 11, 2022

@tianyiw2013 I believe I have fixed the problem with the coverage tests and have updated your branch

@neild3r neild3r changed the title fix full class names Fix full class names Feb 11, 2022
@neild3r neild3r merged commit e81a788 into neild3r:master Feb 11, 2022
@tianyiw2013 tianyiw2013 deleted the full_class_name branch February 14, 2022 01:51
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