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

Add openApp method #263

Merged
merged 5 commits into from
Oct 7, 2021
Merged

Add openApp method #263

merged 5 commits into from
Oct 7, 2021

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Oct 5, 2021

open is good to facilitate cross platform opening. sometimes i just want to open an application without specifying url or file target. the concept is something like open(null, { app: { name: open.apps.chrome }});. this pr just introduces an openApp api. please help to review if the concept makes sense.

index.js Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Add openApp api to open an application Add openApp method Oct 6, 2021
Kudo and others added 2 commits October 6, 2021 23:49
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@Kudo
Copy link
Contributor Author

Kudo commented Oct 6, 2021

thanks @sindresorhus. these review comments are neat and i should update all. please take a look when you get a chance.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved

Type: `Array<string>`
Default: `[]`

Copy link
Owner

Choose a reason for hiding this comment

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

Missing the description from index.d.ts

@sindresorhus
Copy link
Owner

Can you manually verify that these changes do not break normal usage of open?

Kudo and others added 2 commits October 7, 2021 18:21
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@Kudo
Copy link
Contributor Author

Kudo commented Oct 7, 2021

thanks for kindly helping with these wordings.

Can you manually verify that these changes do not break normal usage of open?

i did execute ava to test cases in test.js, anything else recommended to further test?

@sindresorhus
Copy link
Owner

open/test.js

Lines 4 to 7 in e21e623

// Tests only checks that opening doesn't return an error
// it has no way make sure that it actually opened anything.
// These have to be manually verified.

@Kudo
Copy link
Contributor Author

Kudo commented Oct 7, 2021

i did go through the test cases one by one in macos. there are just two cases with pipe not opening anything. that's same with main branch and open does not encode target in macos.

open/test.js

Lines 57 to 63 in e21e623

test('open URL with query strings and pipes', async t => {
await t.notThrowsAsync(open('https://sindresorhus.com/?abc=123&def=456&ghi=w|i|t|h'));
});
test('open URL with query strings, spaces, pipes and a fragment', async t => {
await t.notThrowsAsync(open('https://sindresorhus.com/?abc=123&def=456&ghi=w|i|t|h spaces#projects'));
});

@sindresorhus sindresorhus merged commit 1acc682 into sindresorhus:main Oct 7, 2021
@Kudo Kudo deleted the feat/openApp branch October 7, 2021 16:53
@Kudo
Copy link
Contributor Author

Kudo commented Oct 7, 2021

thank you, @sindresorhus. you are awesome to have these cool libraries.

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