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

Updated .toml & test cases to match problem specifications repo for phone numbers exercise #870

Merged
merged 7 commits into from
May 28, 2024

Conversation

tanno1
Copy link
Contributor

@tanno1 tanno1 commented May 13, 2024

third revision of my first pr here lol. the reason for this pr was originally from issue #775, and contains the updates that @vaeng was asking for in that issue. i references other pr's from that issue in my updates. let me know if there are questions or things i need to change.

Copy link
Contributor

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this May 13, 2024
@tanno1 tanno1 mentioned this pull request May 13, 2024
21 tasks
@vaeng vaeng reopened this May 14, 2024
@vaeng
Copy link
Contributor

vaeng commented May 14, 2024

Thanks for submitting that PR.
You have opened three pull requests for the same task. This is a bit confusing as a maintainer. Is there a reason, why you did not re-use the ones you have started?

Please see the clang formatting suggestions, to pass the initial check.

@tanno1
Copy link
Contributor Author

tanno1 commented May 14, 2024

@vaeng sorry for the confusion with multiple PRs, I didn't know that I could keep updating the same with new pushes, learning a lot. This PR is the only one I will be focusing on now and I will push changes to it.

I updated the formatting, is there a way I can run the initial check? I don't see it happening automatically when I push new code so I am assuming it requires some kind of approval from you?

@tanno1
Copy link
Contributor Author

tanno1 commented May 14, 2024

can you run the reviews again @vaeng, from the root with the .clang-format file I ran "clang-format -i /path/to/test_file.cpp" this time and afaik that should be formatting correctly based off of the .clang-format file?

@tanno1
Copy link
Contributor Author

tanno1 commented May 21, 2024

checks have passed, is there anything else I need to change here before it can be merged @vaeng?

@vaeng
Copy link
Contributor

vaeng commented May 27, 2024

@tanno1 sorry for the delay, I will have time tomorrow to check your commits

Copy link
Contributor

@vaeng vaeng left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@vaeng vaeng merged commit 69e52dd into exercism:main May 28, 2024
8 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