-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Update flush-dns extension #18964
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
Update flush-dns extension #18964
Conversation
Thank you for your contribution! 🎉 🔔 @rasmusbe @tombonez you might want to have a look. You can use this guide to learn how to check out the Pull Request locally in order to test it. You can expect an initial review within five business days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR updates the flush-dns extension to support Windows platform while modifying the DNS cache flushing functionality for both macOS and Windows operating systems.
- Title in
extensions/flush-dns/package.json
should be "Flush DNS" (not "Flush Dns") since DNS is an acronym - The
launchCommand
inextensions/flush-dns/src/index.ts
should be wrapped in a try-catch block - Consider using
showFailureToast
from@raycast/utils
instead of custom error handling inhandleExecutionError
function - Recommend adding a subtitle "System" to the command in
package.json
for better context - Windows-specific error handling in
handleExecutionError
could be simplified by usingshowFailureToast
💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!
2 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
extensions/flush-dns/package.json
Outdated
@@ -1,7 +1,7 @@ | |||
{ | |||
"$schema": "https://www.raycast.com/schemas/extension.json", | |||
"name": "flush-dns", | |||
"title": "Flush DNS", | |||
"title": "Flush Dns", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: 'Flush Dns' should be 'Flush DNS' since DNS is an acronym
"title": "Flush Dns", | |
"title": "Flush DNS", |
extensions/flush-dns/src/index.ts
Outdated
switch (osPlatform) { | ||
case "darwin": | ||
{ | ||
const macResult = getMacOSCommands(osCommandSet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: getMacOSCommands is called without await but returns a Promise
const macResult = getMacOSCommands(osCommandSet); | |
const macResult = await getMacOSCommands(osCommandSet); |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition! I'm unable to try the Windows part it since I only own a mac, but it looks valid. 👏
I haven't verified that the mac parts still work, I will be able to do that on Thursday since I'm not having access to my development computer until then but I've added some small feedback based on what I see in the code.
extensions/flush-dns/package.json
Outdated
"title": "Flush DNS", | ||
"title": "Flush Dns", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Flush Dns' should be 'Flush DNS' since DNS is an acronym
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linter was complaining, that why i changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look into this, let's use DNS for sure :)
commands: { | ||
ipconfig: "ipconfig /flushdns", | ||
}, | ||
shell: undefined, // Uses cmd.exe by default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be undefined or "cmd.exe"
in that case. Feels like cmd.exe is making more sense, but I don't have a strong opinion regarding this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raycast windows don't have showHUD api right so hard to know if throwing error, but Nodejs docs says right here node uses process.env.ComSpec as default shell as cmd.exe
Co-authored-by: Rasmus <[email protected]>
i dont see any lint error locally is ci bugged or something? @pernielsentikaer |
Should be better now @c0b41 - I'll leave this until we have support for everything that this extension uses |
everything working on windows side, but someone should test macos stuff again, i hope i didn't break anything |
@rasmusbe did you have a chance to test it yet? |
can someone test this changes |
This will be reviewed by our Windows team when they are ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Additional updates to the Flush DNS extension's Windows support implementation and documentation.
- The
CHANGELOG.md
entry correctly follows format with{PR_MERGE_DATE}
template and proper placement at the top - While adding Windows support in
package.json
, maintainers should review extensive Windows-specific code changes inindex.ts
for platform-specific command execution - Code modularization with separate functions improves maintainability but needs security review for Windows command execution
3 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, I have tested it now on Mac, and it still works as it should. Great job @c0b41!
Published to the Raycast Store: |
🎉 🎉 🎉 We've rewarded your Raycast account with some credits. You will soon be able to exchange them for some swag. |
Description
Screencast
Checklist
npm run build
and tested this distribution build in Raycastassets
folder are used by the extension itselfREADME
are located outside the metadata folder if they were not generated with our metadata tool