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

from-ssv: user-defined number of spaces to split on #835

Merged
merged 9 commits into from Oct 15, 2019

Conversation

thomasheartman
Copy link

As mentioned in #830, this is an extension of the basic from-ssv command. The command now takes an optional argument to let the user decide how many spaces to use as a minimum for separation.

Current usage: from-ssv -n <usize>

Discussion points

There are a few things I'd like to get some input on before getting this merged:

How to pass the value

  • Should it be a flag (as it's implemented now), or should it behave like first, where you can simply type the command followed by a number?

  • What should the flag be called? I tried to use a more descriptive name, something like allowed-spaces or the like, but ran into various errors when the input was being parsed, regarding type errors, expecting booleans, integers, or getting flags. In the end, this was the only thing I got working. If we want something more verbose, I think I'd need some help to get it implemented. That said, I like that it's a short flag.

@gitpod-io
Copy link

gitpod-io bot commented Oct 15, 2019

@thomasheartman
Copy link
Author

Also, I've set the default minimum number of spaces to count as a separator to 1 (which is the same as it was previously), but this is a good time to consider whether that's a sensible default or not. I don't know how other people usually use this format, but for my use case, so I can't speak for anyone else, but for me, it would be most useful if the default was 2 or 3 spaces. Any thoughts on this?

@thomasheartman thomasheartman marked this pull request as ready for review October 15, 2019 19:38
@sophiajt
Copy link
Member

I think it should be a flag. I think we should start with a full flag name for now. In the future, we'll have support for short flag names we can use as well.

I'd say something like --minimum-spaces x. Could you show an example of something that didn't work for you?

@thomasheartman
Copy link
Author

So I got it to work by looking around a bit in the code. It seems that using the attribute

#[serde(rename(deserialize = "minimum-spaces"))]

it works just fine, but without it I can't seem to make it work. The annotation was missing earlier, so that's probably it.

Anyway, updated the flag now. Also bumped the default minimum separator to two. From a quick look around, I couldn't find any spec on what the format is (don't know if there is an actual format), so I thought it'd make more sense to adapt it to the (my?) primary use case, which in this case is parsing kubernetes output.

@sophiajt
Copy link
Member

Yeah, that might be the trick. Getting the name to match up using the serde flag like that is fine with me.

So, with that, are we good to land?

@thomasheartman
Copy link
Author

Just to updated the readme with the name of the flag and added some more information to the usage string, so yeah, we should be all good now!

README.md Outdated Show resolved Hide resolved
@sophiajt
Copy link
Member

Looks good!

@sophiajt sophiajt merged commit 7d30251 into nushell:master Oct 15, 2019
bobhy pushed a commit to bobhy/nushell that referenced this pull request Oct 22, 2023
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.

None yet

2 participants