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

fix: Implement some logic for isExecutable for windows #3152

Merged
merged 1 commit into from
Aug 6, 2023

Conversation

arran4
Copy link
Contributor

@arran4 arran4 commented Aug 5, 2023

This is based on:

fix: Provide some logic for isExecutable under windows for determining if it's an executable

BREAKING CHANGE: isExecutable no longer always returns false and tries to determine execution status by file extension

…ing if it's an executable

BREAKING CHANGE: `isExecutable` no longer always returns false and tries to determine execution status by file extension
@arran4 arran4 changed the title Implement some logic for isExecutable for windows fix: Implement some logic for isExecutable for windows Aug 5, 2023
Copy link
Collaborator

@halostatue halostatue left a comment

Choose a reason for hiding this comment

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

This looks good to me. I’m going to defer approval to @twpayne because it is a breaking change, but I think that it’s a sensible change. I do not expect that anything is depending on isExecutable returning false on Windows, but Hyrum’s law always applies.

@arran4
Copy link
Contributor Author

arran4 commented Aug 6, 2023

I think it is primarily used for one of the source control systems. In which case it might need to be split.

Defining the correct level of abstraction is a completely separate discussion (see Mythical Man-Month)

I don't remember that being in the book, but means I can refer to it a lot more. The 70s man..

@twpayne
Copy link
Owner

twpayne commented Aug 6, 2023

Thanks, this makes sense. There are few minor changes I'd like to do to make the control flow more compact, but I'll do this in a separate PR.

@twpayne twpayne merged commit 8c9905c into twpayne:master Aug 6, 2023
19 checks passed
@arran4
Copy link
Contributor Author

arran4 commented Aug 6, 2023

Thanks I will read.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants