Skip to content

Conversation

tectiv3
Copy link
Contributor

@tectiv3 tectiv3 commented Mar 23, 2020

Hi!
I've extracted API Resource related changes from my fork into this PR.

Please take a look, does everything makes sense?

Tried to keep unrelated changes to the minimum, had to add prettier config so my editor wouldn't reformat the whole file on every save :)

@tectiv3 tectiv3 marked this pull request as ready for review March 23, 2020 23:57
@jasonmccreary
Copy link
Collaborator

This code looks good, but again, with no tests, I can't merge such a large feature.

@jasonmccreary
Copy link
Collaborator

Also, please remove the .prettier and .gitignore changes. These are for your system and not this project.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Mar 25, 2020

Also, please remove the .prettier and .gitignore changes. These are for your system and not this project.

Okay! No problem, thought it might help other contributors.

I’m really not good at writing tests but I’ll try to follow along.

@jasonmccreary
Copy link
Collaborator

If it were project specific stuff, sure. This project already uses PHP CS and Style CI. So there's no reason to add Prettier. And .DS_Store is MacOS specific and should be part of your global ignore anyway.

As far as the tests, it's okay if you don't have full coverage.

Maybe try to add one for the ResourceStatement and one for the expected API controller output. You should be able to mimic the existing tests to write those with minimal work. It's mostly set up of the fixtures for the input and expected output.

tectiv3 added 2 commits March 26, 2020 10:27
# Conflicts:
#	src/Generators/ControllerGenerator.php
#	src/Lexers/StatementLexer.php
@defenestrator
Copy link

I’m really not good at writing tests but I’ll try to follow along.

I'm happy to help write some tests for this, if you would like. I'm digging this tool and your commit could be very helpful to me.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Mar 26, 2020

I’m really not good at writing tests but I’ll try to follow along.

I'm happy to help write some tests for this, if you would like. I'm digging this tool and your commit could be very helpful to me.

by all means!

@defenestrator
Copy link

I'm going to work on this today.

Copy link

@defenestrator defenestrator left a comment

Choose a reason for hiding this comment

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

I haven't read the source of blueprint until now, but your changes look pretty clean, I need to make sure the tests are up to JMac standards and I honestly haven't learned his approach at all before today, but I should be able. Really a good PR for my uses, personally, nice work!

@jasonmccreary
Copy link
Collaborator

@defenestrator no worries. Take a crack at it and I can always clean it up. Maybe a live-stream Friday.

@jasonmccreary
Copy link
Collaborator

I started working on this earlier. Due to GitHub's limitations for working on a contributors branch directly, I am going to close this and will open a new one with your work + mine.

@defenestrator
Copy link

defenestrator commented Apr 4, 2020

I didn't know there were GitHub limitations that would cause an issue, interesting. I had other priorities, had to delay this, dang it. I have a handle on your testing stuff, dig those fixtures. I'm checking out that new branch now.

@jasonmccreary
Copy link
Collaborator

No worries. If you have changes you did, you can try to PR them into the branch I made.

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.

3 participants