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

Remove ASSOC and FTYPE from allowed CMD builtins #6441

Closed
wants to merge 2 commits into from

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Aug 29, 2022

They no longer function as expected on Win8+: https://stackoverflow.com/a/51727990. As such, exposing them through nu doesn't make any sense.

I confirmed on my Win11 machine that assoc/ftype show a file type association from .html to IE, despite the actual association on my machine being to Firefox.

cc @rgwood

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace --features=extra to check that all the tests pass

@rgwood
Copy link
Contributor

rgwood commented Aug 29, 2022

Thanks for the find and congrats on the first PR! I think you'll need to change the size of the array too.

@CAD97
Copy link
Contributor Author

CAD97 commented Aug 29, 2022

That's what I get for doing this in the web UI 🙃

@rgwood
Copy link
Contributor

rgwood commented Aug 29, 2022

@fdncred This sounds sensible to me; no rush but how do you feel about it as Windows Guy #1?

@fdncred
Copy link
Collaborator

fdncred commented Aug 29, 2022

Windows Guy #1

LOL!

I was shocked to hear assoc doesn't work any longer. I just used it the other day in Windows 10. I wonder if it's like read-only but not used to set associations? I'd like to try this tomorrow too.

@CAD97
Copy link
Contributor Author

CAD97 commented Aug 29, 2022

@fdncred IIUC, assoc/ftype still do what they always did, it's just that Windows has user-level settings for filetype associations as well now which they don't inspect, and which are the ones that get changed in the GUI.

@fdncred
Copy link
Collaborator

fdncred commented Aug 29, 2022

after looking at this a bit, i'd vote to keep ftype and assoc in the code. i can still use it to query my file types and such. i don't really see what harm it is to have it in the code the way it is now?

@fdncred
Copy link
Collaborator

fdncred commented Sep 3, 2022

I'd like to close this PR since these command are still useful even though they might be more limited these days. Is there going too be any heartburn over that @rgwood @CAD97?

@rgwood
Copy link
Contributor

rgwood commented Sep 3, 2022

No objections from me.

@CAD97
Copy link
Contributor Author

CAD97 commented Sep 3, 2022

Seems reasonable enough to me; while other commands are passed through, there's no real cost to also passing these through.

Long term, I think it'd be beneficial to remove this passthrough entirely and provide native (and somewhat portable) functionality to replace all of the cmd builtins. Perhaps also the default config can include (if OS is windows) an export alias assoc = ^cmd /c assoc for the stragglers.

For now though, I won't push the issue.

@CAD97 CAD97 closed this Sep 3, 2022
@CAD97 CAD97 deleted the patch-1 branch October 8, 2023 19:52
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.

3 participants