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

added prival to string function #413

Merged
merged 39 commits into from
Apr 19, 2024
Merged

added prival to string function #413

merged 39 commits into from
Apr 19, 2024

Conversation

uFarad
Copy link
Contributor

@uFarad uFarad commented Apr 4, 2024

Added function to convert the prival integer to a string in response to issue #245

@mk722 did great work on this previously, but seems to be inactive now after a bout with COVID. I hope they had a speedy recovery.

I took over this change from where they left off. Please let me know if anything needs to be changed or if I am not following proper procedures. I hope to contribute to more open source projects like this one in the future.

@uFarad uFarad changed the title added prival to string function #245 added prival to string function Apr 4, 2024
@uFarad uFarad mentioned this pull request Apr 4, 2024
@uFarad uFarad marked this pull request as draft April 4, 2024 11:14
@uFarad uFarad marked this pull request as ready for review April 5, 2024 03:37
@goatshriek
Copy link
Owner

Note that the continuous integration tests cannot run until the merge conflicts are resolved.

@goatshriek goatshriek self-assigned this Apr 13, 2024
@goatshriek goatshriek added the enhancement new features or improvements label Apr 13, 2024
@uFarad uFarad marked this pull request as draft April 15, 2024 02:48
@uFarad uFarad marked this pull request as ready for review April 15, 2024 03:21
@uFarad
Copy link
Contributor Author

uFarad commented Apr 15, 2024

It successfully compiles and passes tests locally. It should be good to go.

@goatshriek
Copy link
Owner

Thanks very much for getting this together! You can disregard the codecov upload failures, those are due to a separate issue I am working on addressing. I will do a more thorough review of the change in the next few days.

Copy link
Owner

@goatshriek goatshriek left a comment

Choose a reason for hiding this comment

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

See the requested changes for the first round of revisions.

Also, make sure that you run the header check tool and resolve any header errors. The static analysis CI workflow is currently failing on header checks.

include/private/prival.h Outdated Show resolved Hide resolved
include/stumpless/prival.h Outdated Show resolved Hide resolved
include/stumpless/prival.h Outdated Show resolved Hide resolved
include/stumpless/prival.h Show resolved Hide resolved
include/stumpless/prival.h Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@uFarad uFarad requested a review from goatshriek April 18, 2024 01:49
@goatshriek goatshriek merged commit 3cd831e into goatshriek:latest Apr 19, 2024
47 of 53 checks passed
@goatshriek
Copy link
Owner

Thank you once again for working through this! My apologies for the many confusing CI failures - you had the misfortune of committing during a time of codecov breakage and also discovering a bug in the header checking tool.

@uFarad
Copy link
Contributor Author

uFarad commented Apr 20, 2024

It was my pleasure. Thank you for creating an amazing project that is friendly to those new to OSS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new features or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants