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

Correctly parse the output from the bios executable #80

Merged
merged 3 commits into from
Oct 28, 2019

Conversation

jinwoo
Copy link
Contributor

@jinwoo jinwoo commented Oct 24, 2019

Multi-word arguments must be parsed correctly. E.g., currently -flag 'foo bar' is parsed as [-flag, 'foo, bar'].
It should be [-flag, 'foo bar'].

Use the shellwords package for that.

Multi-word arguments must be parsed correctly. E.g., currently `-flag
'foo bar'` is parsed as [-flag, 'foo, bar'].
It should be [-flag, 'foo bar'].

Use the shellwords package for that.
hie-bios.cabal Outdated
@@ -53,6 +53,7 @@ Library
process >= 1.6.1 && < 1.7,
file-embed >= 0.0.10 && < 0.1,
ghc >= 8.2.2 && < 8.9,
shellwords >= 0.1.2 && < 0.2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This adds a megaparsec dependency which I want to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now this PR expects arguments to be newline-delimited. Each argument must be in its own line.

So the only change now is words res -> lines res.

This will break existing users.

@mpickering
Copy link
Collaborator

The solution in the cabal case was to delimit the arguments by \NULL. Would that work here?

@jinwoo
Copy link
Contributor Author

jinwoo commented Oct 25, 2019

The solution in the cabal case was to delimit the arguments by \NULL. Would that work here?

I can do that. But wouldn't it break existing users?

Or I can write my own parsing logic here, but I guess that'll require some kind of parser combinator package anyway to make it simple.

If you're fine with breaking existing users, I can go with the \NULL delimiter solution.

@jinwoo
Copy link
Contributor Author

jinwoo commented Oct 25, 2019

What about using a newline as the delimiter? I assume that's easier for custom bios programs to generate outputs.

-foo arg1 -bar 'multi word arg'

will become

-foo
arg1
-bar
multi word arg

This removes the shellwords package dependency.

Previously the arguments were space-delimited but now they are
newline-delimited. So each argument must be in its own lines.

This is a breaking change.
@mpickering
Copy link
Collaborator

Using newlines seems like quite an ingenious solution. In bash is there a convenient way to convert a list of arguments separated by spaces to one separated by newlines? I am just wondering how to convert the GHCi target in the ghc repo to work if the repl arguments target returns newline rather than space separated arguments.

The documentation also needs updating but a lot of it is out of date anyway.

@jinwoo
Copy link
Contributor Author

jinwoo commented Oct 28, 2019

Made a slight change in the README.md file.

Re: converting spaces to newlines, maybe sed? But if we blindly convert spaces to newlines, things will break again. Maybe convert only when the output contains no newline characters. But it can be also a problem when the output contains only one argument and it is a multi-word argument.

@mpickering
Copy link
Collaborator

I think it will be easier to convert newlines into spaces, so I'll go that direction.

@jinwoo jinwoo deleted the words branch October 28, 2019 17:28
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