Escape Prisma command when running with script on Linux
#1561
Conversation
Signed-off-by: Mihovil Ilakovac <[email protected]>
infomiho
force-pushed
the
miho-path-space
branch
from
b55946d
to
4b375fc
Compare
infomiho
changed the title
script on Linux
waspc/src/Wasp/Util/ShellCommand.hs
Outdated
| @@ -0,0 +1,22 @@ | |||
| module Wasp.Util.ShellCommand where | |||
There was a problem hiding this comment.
This is quite cool, I never used IsString!
I like the concept you used here, it is quite neat. So basically we have this notion of Shell command argument, and we can say if it is quoted or not, and we carry this knowledge around, and then can act on it.
What I found tricky is:
- There are no comments in this file explaining the reasoning behind it and what it is for. It is somewhat obvious, but I don't think it is obvious enough.
- Continuing on above, I think the reason it is not obvious enough is that usecase feels quite trivial. If I want args quoted, why don't I continue using String and just add quotes to it? Ok, I guess this gives more knowledge, so you can avoid quoting already quoted arg, or you can unquote already quoted arg (although that feels a bit weird, why was it quoted then, but ok, maybe we know somebody else will quote it). Still, I think it is a bit confusing so explanation would help.
- Using it above in the Jobs.hs feels a bit like overkill. Basically we need to quote these two args that are paths. And in one situation we need to quote them, in other we don't. But now we have to introduce this new type in all of the args there. But, not all of them, just some of them -> its not like we are using it across the whole codebase, just here. Feels weird that we are using it just in the small part of the codebase, how do we choose? When should I use this? Btw, isn't there a simpler solution -> can't we just quote those paths and that is it? Is it a problem if they are also quoted in that situation where they shouldn't be quoted, or will it just work?
There was a problem hiding this comment.
So, the problem we have is the following: we need the same command in a slightly different ways for Linux and for MacOS.
MacOS
- The arguments should be a list of arguments
- The paths passed into the prisma command MUST NOT be quoted
Linux
- The arguments are concatenated into a single string
- The paths MUST be quoted (since they can have spaces in them)
I'll try to simplify the solution and make it very local (it will only affect the array of prisma db migrate args and nothing else).
There was a problem hiding this comment.
Nice, this is much simpler now!
I left some small comments but otherwise it is good.
I would possibly consider skipping this file here, ShellCommand.hs, and just inlining this function quoteArg where it is used, since it is so niche / local, I don't think we benefit much from having it here in a separate file.
|
@Martinsos I've simplified the impl, tested it Before
After
|
| ["migrate", "status", "--schema", SP.fromAbsFile schema] | ||
| [ "migrate", | ||
| "status", | ||
| "--schema", | ||
| SP.fromAbsFile schema | ||
| ] |
There was a problem hiding this comment.
If you really like it better in multiple lines fine, but I liked it better in one line, found it easier to read. And since it has nothing to do with this PR maybe we could revert it? Same goes for other couple of functions in this file.
| -- NOTE(miho): Since we are running the Prisma command using `script` and we are doing it | ||
| -- in two different ways (MacOS and Linux), we have to take care of quoting the paths | ||
| -- differently. | ||
| -- * MacOS - we are passing the command as multiple arguments, so we MUST NOT quote the paths. | ||
| -- * Linux - we are passing the command as one argument, so we MUST quote the paths. | ||
| buildPrismaMigrateCmd quoteArg = | ||
| [ quoteArg $ absPrismaExecutableFp projectDir, |
There was a problem hiding this comment.
| -- NOTE(miho): Since we are running the Prisma command using `script` and we are doing it | |
| -- in two different ways (MacOS and Linux), we have to take care of quoting the paths | |
| -- differently. | |
| -- * MacOS - we are passing the command as multiple arguments, so we MUST NOT quote the paths. | |
| -- * Linux - we are passing the command as one argument, so we MUST quote the paths. | |
| buildPrismaMigrateCmd quoteArg = | |
| [ quoteArg $ absPrismaExecutableFp projectDir, | |
| -- NOTE(miho): Since we are running the Prisma command using `script` and we are doing it | |
| -- in two different ways (MacOS and Linux), we have to take care of quoting the path arguments | |
| -- differently. | |
| -- * MacOS - we are passing the command as multiple arguments, so we MUST NOT quote the path arguments. | |
| -- * Linux - we are passing the command as one argument, so we MUST quote the path arguments. | |
| buildPrismaMigrateCmd :: (String -> String) -> [String] | |
| buildPrismaMigrateCmd argQuoter = | |
| [ argQuoter $ absPrismaExecutableFp projectDir, |
waspc/src/Wasp/Util/ShellCommand.hs
Outdated
| @@ -0,0 +1,22 @@ | |||
| module Wasp.Util.ShellCommand where | |||
There was a problem hiding this comment.
Nice, this is much simpler now!
I left some small comments but otherwise it is good.
I would possibly consider skipping this file here, ShellCommand.hs, and just inlining this function quoteArg where it is used, since it is so niche / local, I don't think we benefit much from having it here in a separate file.
Closes #1486