Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

chore: aesthetic updates to tests #248

Merged
merged 3 commits into from
Dec 24, 2022
Merged

Conversation

KennedyTedesco
Copy link
Contributor

Hi everyone!

I'm excited to make my first contribution to the project. I recently started learning about parsers and interpreters, and I've been really interested in exploring the subject further. As a beginner in both the subject and Rust, working on this project is definitely a great way to learn.

I noticed that there were some areas of the codebase where the style and formatting could be improved, so I took the time to make some changes.

Are you guys open to accepting this kind of change?

Cheers!

@azjezz
Copy link
Collaborator

azjezz commented Dec 24, 2022

Hey @KennedyTedesco

Yes, we are open to all kinds of contribution.

However, we prefer that these type of changes are done in one PR, so if you are planning on doing more similar fixes, please append them to this PR instead of creating a new one, and let me know when it's ready for review.

@KennedyTedesco
Copy link
Contributor Author

Sure, I'll make other tweaks in the test suite. I'll let you know. o/

@KennedyTedesco
Copy link
Contributor Author

Done!

I've made some changes by separating the reading and processing of the test. However, being honest, I'm not sure if this type of refactoring is necessary or desired. My approach may not align with the goals of the project. If you agree, I will focus on bug fixes and new features instead of refactoring things in the future to avoid causing any issues with review processes.

Feel free to close the PR if it brings much noise to the code base. 😊

@ryangjchandler
Copy link
Contributor

Bug fixes and features are preferred, we can focus on tidy up etc once we're "stable".

@KennedyTedesco
Copy link
Contributor Author

I agree with this perspective. ✌️

Copy link
Collaborator

@azjezz azjezz left a comment

Choose a reason for hiding this comment

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

I think this is okay to merge, only one issue.

@azjezz azjezz merged commit 055f8fe into php-rust-tools:main Dec 24, 2022
@KennedyTedesco KennedyTedesco deleted the tests branch December 24, 2022 22:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants