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

impl FromStr for sys::Signal #884

Merged
merged 1 commit into from Sep 5, 2018
Merged

Conversation

@quodlibetor
Copy link
Contributor

@quodlibetor quodlibetor commented Apr 15, 2018

This implements both ALLCAPS and lowercase full name (including SIG) and short
name.

This matches what kill accepts in many shells, and it also allows the Debug
representation of Signal, which means it can be round-tripped, if desired.

@quodlibetor
Copy link
Contributor Author

@quodlibetor quodlibetor commented Apr 16, 2018

The failure looks spurious:

error: failed to build archive: No space left on device
@asomers
Copy link
Member

@asomers asomers commented Apr 16, 2018

I fixed the ENOSPC and restarted the builds.

@asomers
Copy link
Member

@asomers asomers commented Apr 16, 2018

I don't agree with this change. If nix were an interactive shell it would make sense. But nix is a library. Why would we want to intentionally broaden our API by publishing multiple spellings of common symbols? I've worked on projects that did things like this. I just creates inconsistency and confusion, not to mention more maintenance.

@quodlibetor
Copy link
Contributor Author

@quodlibetor quodlibetor commented Apr 16, 2018

Fair enough. Would you be more receptive to a change that only accepts a subset of these spellings? For example, just the exact matches (only SIGHUP, not SIGHUP and HUP and sighup and hup)? I'm happy to adjust. This is also just a way to reduce boilerplate and a newtype in one of my projects so I wouldn't be terribly upset if you just want to reject this.

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Apr 16, 2018

I don't like that there isn't a ToStr implementation to go with this. We should either have both or neither. I'm also of the opinion that there should only be one canonical representation of each signal, the full capitalized version.

@quodlibetor What's your use case here? You want to parse command-line arguments specifying signals or are these strings in a data stream?

@quodlibetor
Copy link
Contributor Author

@quodlibetor quodlibetor commented Apr 16, 2018

@Susurrus I'm happy to implement Display and AsRef<str> for these. My use case is command-line flags (here).

This PR verifies that the Debug impl does roundtrip, which isn't as neat as adding to_string but does work. I agree that round-tripping is important.

I would slightly prefer to keep both all-caps versions (e.g. SIGHUP and HUP), but that's just because I think of them as synonyms and I would be happy to remove everything if coming in with a limited feature spec would be more likely to get accepted.

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Apr 18, 2018

What I would suggest is that we only support the SIG[A-Z]* format for signal identifiers. Additionally we implement Display for Signal and only test that that can be round-tripped. Additionally our round-tripped tests should do it for every variant and some ones that are clearly erroneous, rather than testing just a singular one.

@quodlibetor
Copy link
Contributor Author

@quodlibetor quodlibetor commented Apr 18, 2018

I've done most of that. I implemented AsRef<str> as well, because it's possible to implement Display in terms of AsRef so the total amount of code is almost the same, but with all the benefits of not needing to allocate for the common-case in rust of just needing an &str.

@quodlibetor
Copy link
Contributor Author

@quodlibetor quodlibetor commented Apr 18, 2018

I would still prefer to include the short-all-caps names, but if you're happy with this then so am I.

@quodlibetor quodlibetor force-pushed the quodlibetor:fromstr-for-signal branch from 9aa5472 to d5db6f7 Apr 20, 2018
@Mange
Copy link

@Mange Mange commented Sep 3, 2018

I'd love to see this shipped. Is there anything I can help with?

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Sep 4, 2018

@Mange Take it for a spin in your own code, feel free to leave a review or feedback.

@Susurrus Susurrus requested a review from asomers Sep 4, 2018
@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Sep 4, 2018

This looks good to me at this point. @asomers any comments?

Before merging, it does need the following:

  • Probably a rebase
  • Squashed into a single commit with a good commit message
  • A CHANGELOG "Added" entry
Copy link
Member

@asomers asomers left a comment

I agree with susurrus. It looks good except for the CHANGELOG entry and the squash.

@quodlibetor quodlibetor force-pushed the quodlibetor:fromstr-for-signal branch from d5db6f7 to ec8f415 Sep 5, 2018
This is a subset of what `kill` accepts in many shells. The Display
implementation matches the `Debug` representation of `Signal`.

The `FromStr` matches both Debug/Display which means it can be round-tripped,
if desired.
@quodlibetor quodlibetor force-pushed the quodlibetor:fromstr-for-signal branch from ec8f415 to 8de86cc Sep 5, 2018
@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Sep 5, 2018

bors r+

bors bot added a commit that referenced this pull request Sep 5, 2018
Merge #884
884: impl FromStr for sys::Signal r=Susurrus a=quodlibetor

This implements both ALLCAPS and lowercase full name (including SIG) and short
name.

This matches what `kill` accepts in many shells, and it also allows the `Debug`
representation of `Signal`, which means it can be round-tripped, if desired.

Co-authored-by: Brandon W Maister <quodlibetor@gmail.com>
@bors bors bot merged commit 8de86cc into nix-rust:master Sep 5, 2018
4 checks passed
4 checks passed
bors Build succeeded
Details
buildbot/nix-rust/nix amd64_fbsd11 Build done.
Details
buildbot/nix-rust/nix i386_fbsd11 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mpasternacki added a commit to mpasternacki/nix that referenced this pull request Nov 16, 2018
bors bot added a commit that referenced this pull request Nov 16, 2018
Merge #973
973: CHANGELOG: move entry from #884 from 0.11.0 to [Unreleased] r=asomers a=mpasternacki

PR #884 added changelog entry in section for 0.11.0, which has been already released, instead of in the [Unreleased] section (which wasn't present yet). Moved the entry where it belongs.

Co-authored-by: Maciej Pasternacki <maciej@3ofcoins.net>
levex added a commit to levex/nix that referenced this pull request Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants