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

terraformer planet field bonus logic #412

Merged
merged 6 commits into from
Oct 25, 2024

Conversation

jackbayliss
Copy link
Contributor

This should handle #392

Also adds a variety of tests - feel free to let me know any adjustments.

Copy link
Owner

@lanedirt lanedirt left a comment

Choose a reason for hiding this comment

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

Hi @jackbayliss,

Changes look good to me! I just have a few small whiteline nitpicks to make it a little bit cleaner and conform to the rest of the codebase. Could you take a look at these?

Other than these minor style tweaks, functionality looks to be working great. Thanks for your work on this! 🚀

app/Services/PlanetService.php Show resolved Hide resolved
tests/Unit/PlanetServiceTest.php Outdated Show resolved Hide resolved
tests/Unit/PlanetServiceTest.php Outdated Show resolved Hide resolved
tests/Unit/PlanetServiceTest.php Show resolved Hide resolved
@jackbayliss jackbayliss requested a review from lanedirt October 24, 2024 20:24
@jackbayliss
Copy link
Contributor Author

@lanedirt Should hopefully now be sorted, did spot a previous test I did where I missed a whitespace after the closing brace, so sorted that too.

Copy link
Owner

@lanedirt lanedirt left a comment

Choose a reason for hiding this comment

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

Hi @jackbayliss,

Thanks for the refactor! In the latest diff, I noticed a duplicate blank line that still remains. Could you take a look at this?

By the way, I'll try to see if I can incorporate these style checks into the PSR/PhpStan code analysis so that the build process can automatically detect them. That should make things easier going forward.

Thanks again for your work and time 😄 !

tests/Unit/PlanetServiceTest.php Outdated Show resolved Hide resolved
@jackbayliss
Copy link
Contributor Author

@lanedirt Good catch, must of missed it- I've now adjusted it.

It would be ideal if we could add this into the code analysis as I guess everyone has their own style 😃

Copy link
Owner

@lanedirt lanedirt left a comment

Choose a reason for hiding this comment

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

LGTM!

@jackbayliss
Copy link
Contributor Author

Not sure why the test failed! Looks to be a bit flaky, but I believe this is already in another issue?

@lanedirt
Copy link
Owner

lanedirt commented Oct 25, 2024

Not sure why the test failed! Looks to be a bit flaky, but I believe this is already in another issue?

Yes it looks like it's the same issue as reported in #401. Have to look into fixing that one soon. 🐛 Re-running tests now.

Edit: tests succeeded. Merging now 🚀 !

@lanedirt lanedirt merged commit ddde918 into lanedirt:main Oct 25, 2024
6 checks passed
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