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

450 stumpless get priority string #476

Merged

Conversation

m3g4d1v3r
Copy link
Contributor

This is a continuation from the other opened PR #457.

Since I don't have the permissions to commit to the other contributor's fork, I've decided to cherry pick them and publish a new PR.

I hope this is the proper way to do it.

Please, let me know if any adjustments are needed in this branch.

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (b638f77) to head (349de16).
Report is 2 commits behind head on latest.

Additional details and impacted files
@@            Coverage Diff             @@
##           latest     #476      +/-   ##
==========================================
- Coverage   90.40%   90.22%   -0.18%     
==========================================
  Files          47       47              
  Lines        4388     4410      +22     
  Branches      588      592       +4     
==========================================
+ Hits         3967     3979      +12     
- Misses        286      288       +2     
- Partials      135      143       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Since the macros always have underscore in them, the NULL verification is
not needed.
@goatshriek goatshriek self-assigned this Nov 9, 2024
@goatshriek goatshriek added the enhancement new features or improvements label Nov 9, 2024
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.

Excellent work! There is one last change needed: please add a test that passes and invalid prival in, and add some error handling to ensure the response is sane. I personally think a NULL response in this case makes the most sense, but since the facility and severity string functions do not do this, I will defer to you for this change. Before the v3.0.0 release I will likely refactor all of them to return NULL in the event of an error so that the scenario is more clear.

The deadlock was not related to your changes, and is the second time I've seen it. I created #477 to capture the logs and remind myself to go back and look at this, but for now I only mention it to you as something you don't need to worry about.

@m3g4d1v3r
Copy link
Contributor Author

I've just added a commit with invalid privals checking and testing.

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.

Just a tweak on how an invalid prival is checked, and this should be good to go.

src/prival.c Outdated Show resolved Hide resolved
@m3g4d1v3r m3g4d1v3r force-pushed the 450-stumpless-get-priority-string branch 2 times, most recently from b4e84b0 to 5a72426 Compare November 9, 2024 13:49
@m3g4d1v3r m3g4d1v3r force-pushed the 450-stumpless-get-priority-string branch from 5a72426 to 349de16 Compare November 9, 2024 14:15
@goatshriek goatshriek merged commit 9172b08 into goatshriek:latest Nov 9, 2024
55 of 56 checks passed
@goatshriek
Copy link
Owner

Thanks very much for finishing the resolution for this one!

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.

3 participants