Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

feat(args): convert embedded and braced variables in command args #86

Merged
merged 4 commits into from
Mar 14, 2017
Merged

feat(args): convert embedded and braced variables in command args #86

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
371ac46
feat(args): convert variables embedded in command args
Mar 14, 2017
a53f3e6
feat(args): convert braced variables in command args
Mar 14, 2017
68bf464
style(args): full test name on one line
Mar 14, 2017
3ddf846
refactor(args): make commandConvert definitly stateless
Mar 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions src/command.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import isWindows from 'is-windows'

export default commandConvert

const envUseUnixRegex = /\$(\w+)/ // $my_var
const envUseWinRegex = /%(.*?)%/ // %my_var%
const envUseUnixRegex = /\$(\w+)|\${(\w+)}/g // $my_var or ${my_var}
const envUseWinRegex = /%(.*?)%/g // %my_var%
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to be concerned about the g here based on your last PR? Perhaps we could just inline these regex declarations within the commandConvert function. I'm sure there's no relevant perf issues with that, and we could avoid the stateful nature of g.

Copy link
Collaborator Author

@hgwood hgwood Mar 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The issue from my last PR arises from the use of g and RegExp.prototype.exec. So here it is OK to use g because exec is not used anymore, and the test I added ensures the problem does not return in the future. That said, I also think inlining would be even safer, so I'll do that. Would you prefer that I keep them as variable inside commandConvert or that I inline them directly in the ternary on line 15?

Copy link
Owner

Choose a reason for hiding this comment

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

Keep them as a variable. That'll help it be self-documenting 👏


/**
* Converts an environment variable usage to be appropriate for the current OS
Expand All @@ -13,9 +13,5 @@ const envUseWinRegex = /%(.*?)%/ // %my_var%
function commandConvert(command) {
const isWin = isWindows()
const envExtract = isWin ? envUseUnixRegex : envUseWinRegex
const match = envExtract.exec(command)
if (match) {
command = isWin ? `%${match[1]}%` : `$${match[1]}`
}
return command
return command.replace(envExtract, isWin ? '%$1$2%' : '$$$1')
Copy link
Owner

Choose a reason for hiding this comment

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

Nice!

}
27 changes: 27 additions & 0 deletions src/command.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,30 @@ test(`is stateless`, () => {
isWindowsMock.__mock.returnValue = true
expect(commandConvert('$test')).toBe(commandConvert('$test'))
})

test(`converts embedded unix-style env variables usage for windows`, () => {
isWindowsMock.__mock.returnValue = true
expect(commandConvert('$test1/$test2/$test3')).toBe(
'%test1%/%test2%/%test3%',
)
})

test(`converts embedded windows-style env variables usage for linux`, () => {
isWindowsMock.__mock.returnValue = false
expect(commandConvert('%test1%/%test2%/%test3%')).toBe(
'$test1/$test2/$test3',
)
})

test(`
Copy link
Owner

Choose a reason for hiding this comment

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

I'm guessing you had to do this because of linting? How about instead you just make it one line and add:

// eslint-disable-next-line max-len

On the line above this one. Would that be ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.

leaves embedded variables unchanged
when using correct operating system`, () => {
isWindowsMock.__mock.returnValue = false
expect(commandConvert('$test1/$test2/$test3')).toBe('$test1/$test2/$test3')
})

test(`converts braced unix-style env variable usage for windows`, () => {
isWindowsMock.__mock.returnValue = true
// eslint-disable-next-line no-template-curly-in-string
Copy link
Owner

Choose a reason for hiding this comment

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

Haha, one of the few cases where this rule needs to be disable 😆

expect(commandConvert('${test}')).toBe('%test%')
})