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

feat: Added basic support for long argument strings #25

Merged
merged 3 commits into from Oct 28, 2021
Merged

feat: Added basic support for long argument strings #25

merged 3 commits into from Oct 28, 2021

Conversation

ghost
Copy link

@ghost ghost commented Oct 26, 2021

Description

First draft at addressing issue #22 .

Looks for an opening quotation mark in the first index of the command argument array. If one is found, the matching closing quotation is found and the enclosing string is joined into a single long argument. The input echo "test this" prints test this.

Handles quotations around a single word as well (e.g. echo "test").

Does not handle single quotes ('') at all.

Does not handle multiple instances of long arguments

How is this looking so far? Should this be placed into a its own function in the same file? Should I try to implement single quotes and multiple long arguments before this is merged?

Fixes #22 (issue)

Pre-review checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if required)
  • My contribution is awesome

@mkrl mkrl self-assigned this Oct 27, 2021
Copy link
Owner

@mkrl mkrl left a comment

Choose a reason for hiding this comment

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

I would say parsing it this way overcomplicates things a little and doesn't really scale up that good.
We could probably use something like this:

  1. Get the actual command from the input: single word before the first space
  2. Use regex to get the rest of the arguments from what's left: something like ("[^"]+"|[^\s"]+) would work great.

That will further reduce the code size and will look more tidy IMO.

@ghost
Copy link
Author

ghost commented Oct 27, 2021

Thanks for the suggestion! That is a much cleaner way of doing this.

As far as I can see this is now handling all but one of the edge cases I could think of. Double and single quotes are handled. Double quotes can be inside single quotes and vice versa. Multiple quoted arguments are handled, as well as a mix of quoted arguments and single-word arguments.

The only edge case I could think of that this doesn't handle is nested quotes of the same type, ie echo 'there's a contraction in the string'. In this case, presumably the user could just use double quotes instead, but if you want to be able to handle this I could try to edit the regex so that the user could escape inner quotes, ie echo 'there\'s a contraction in the string'.

Copy link
Owner

@mkrl mkrl left a comment

Choose a reason for hiding this comment

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

Great job! 👍
The only edge case I could find is words that contain quote marks without the closing pair will be correctly parsed, but the quotation marks will disappear from the output echo "foo => foo.
I don't think it's a big deal, perhaps it could be addressed in a separate PR.

I have left a suggestion that I think will make it even better, but feel free to skip it if you don't feel like using it.

Comment on lines 6 to 12
const splitCommand = cmd.split(' ')
const command = splitCommand[0]
const commandArguments = splitCommand.slice(1)

const commandArgumentsString = splitCommand.slice(1).join(' ')
const argRegex = /('[^']+'|"[^"]+"|[^\s'"]+)/g
const argMatches = commandArgumentsString.match(argRegex)
const commandArguments = (argMatches === null ? [] : argMatches)
.map(element => element.replace(/(^['"]|['"]$)/g, ''))
Copy link
Owner

Choose a reason for hiding this comment

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

That's great!
The only thing that just came to my mind is - we could make this even smaller by getting rid of some variables that have only been used once and doing a shorter array destructing.

I have also included a tiny change to avoid .map pass over an empty array.

  const [command, ...args] = cmd.split(' ')
  const argMatches = args.join(' ').match(/('[^']+'|"[^"]+"|[^\s'"]+)/g)
  const commandArguments = argMatches === null
    ? []
    : argMatches
      .map(element => element.replace(/(^['"]|['"]$)/g, ''))

(can't make a suggestion because it includes removed lines)

The size difference is ~10 bytes in the final build, but every byte counts :)

Copy link
Author

Choose a reason for hiding this comment

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

Changes made as suggested, thanks for the input! I didn't know you could do that kind of array destructing in js, that's a neat trick.

@mkrl mkrl merged commit 97fb6a0 into mkrl:master Oct 28, 2021
@github-actions
Copy link

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mkrl mkrl added the enhancement New feature or request label Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support command arguments with spaces
1 participant