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

Support for Windows Hidden Files #225

Merged
merged 2 commits into from
Sep 14, 2023
Merged

Conversation

alamb3142
Copy link
Contributor

@alamb3142 alamb3142 commented Sep 8, 2023

This PR adds a step into Files::next_visible_file to check for the hidden attribute on Windows systems. This causes it to behave more like a Windows user would expect it to.

Previously eza only hid dot files when not specifying -a or -A, it now hides dot files and all hidden files. Some types of links are hidden (file junctions are, not sure about hard links) because they're marked as hidden, but symlinks still display with or without the all flag.

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

The commit summary needs to conform to conventional commits. See this for more info.

@alamb3142
Copy link
Contributor Author

alamb3142 commented Sep 8, 2023

Alternative to unpacking & repacking:

                #[cfg(windows)]
                if !self.dotfiles && file.as_ref().is_ok_and(|f| f.attributes().hidden) {
                    continue;
                }

                return Some(file);

May close this PR so I can start again with commits that adhere to the guidelines above

@cafkafk
Copy link
Member

cafkafk commented Sep 8, 2023

May close this PR so I can start again with commits that adhere to the guidelines above

That is an option but shouldn't be nescesarry, you can git rebase -i <commit before yours> and reword them. Then you just have to force push and it will be fixed.

@alamb3142 alamb3142 force-pushed the main branch 3 times, most recently from 8d81aed to b615d04 Compare September 8, 2023 12:32
@alamb3142
Copy link
Contributor Author

Rebased & edited the commit to follow guidelines. Also implemented the change suggested by cfxegbert

@alamb3142
Copy link
Contributor Author

@cafkafk I let GitHub merge the latest changes in, would you rather I rebase again so it's just the one commit?

@PThorpe92
Copy link
Member

PThorpe92 commented Sep 8, 2023

No worries, on merge, github gives you an option to remove it from commit history 👍 pretty sure automatically

@alamb3142 alamb3142 force-pushed the main branch 2 times, most recently from 51eecb3 to 844c0bf Compare September 9, 2023 10:04
@alamb3142 alamb3142 requested a review from cafkafk September 9, 2023 10:05
@alamb3142 alamb3142 force-pushed the main branch 2 times, most recently from 6b99d99 to 34875bf Compare September 11, 2023 09:30
@cafkafk cafkafk marked this pull request as draft September 11, 2023 11:11
@cafkafk
Copy link
Member

cafkafk commented Sep 11, 2023

I marked this as draft. Feel free to make it "ready for review" when you want us to take a look at it again.

@alamb3142
Copy link
Contributor Author

Hey @cafkafk I'm ready for the code to be reviewed, it looks like it's just blocked by the comment about commit messages, which has been addressed

@PThorpe92
Copy link
Member

PThorpe92 commented Sep 13, 2023

Has this been tested on windows? @daviessm sorry to keep pinging you for windows stuff but you kinda brought that on yourself by being the only one with a windows box 😅 it is super appreciated tho ❤️ cuz having to deal with windows just isn't any fun
(btw you should come hang out on the gitter channel)

Otherwise, LGTM 👍

@PThorpe92 PThorpe92 requested a review from daviessm September 13, 2023 14:14
Copy link
Contributor

@daviessm daviessm left a comment

Choose a reason for hiding this comment

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

Tested working on Windows, code looks sensible. Just one minor spelling nit in comments.

src/fs/dir.rs Outdated Show resolved Hide resolved
Adds an extra check on attributes().hidden on Windows
This hides Windows hidden files whenever dot files are also filtered out

Resolves eza-community#212
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

CI and Integration tests pass, and local testing confirms this doesn't break linux so LGTM 👍

@cafkafk cafkafk merged commit e00654b into eza-community:main Sep 14, 2023
@cafkafk
Copy link
Member

cafkafk commented Sep 14, 2023

as pointed out by @cfxegbert on element, is_ok_and was first stabilized in 1.70 (rust-lang/rust#110019), but we have to support 1.65 for gentoo, so unfortunately this will have to be reverted temporarily

@cafkafk
Copy link
Member

cafkafk commented Sep 14, 2023

@alamb3142 could you make a new pull request with the changes, then we can look into changing is_ok_and?

@alamb3142
Copy link
Contributor Author

@cafkafk Yeah I'm happy to start a new PR, but is there any real risk with is_ok_and given that it's only called inside a #[cfg(windows)] block? Is that not sufficient to stop it from breaking on 1.65 Linux builds?

@cafkafk
Copy link
Member

cafkafk commented Sep 14, 2023

@cafkafk Yeah I'm happy to start a new PR, but is there any real risk with is_ok_and given that it's only called inside a #[cfg(windows)] block? Is that not sufficient to stop it from breaking on 1.65 Linux builds?

I mean, currently we specify MSRV as 1.65, and any windows user that below 1.70 will experience this right?

But also, we're discussing bumping the MSRV on element right now, maybe this isn't an issue after all, but to be determined.

@alamb3142
Copy link
Contributor Author

Ah yeah that is a good point. I know most Windows users would prefer Scoop over Cargo for an install, but that's still potentially breaking it for some Windows users. Maybe I could commit the version that unpacks the result & repacks it, then submit a PR with this version for later when the MSRV is bumped?

@cafkafk
Copy link
Member

cafkafk commented Sep 14, 2023

Ah yeah that is a good point. I know most Windows users would prefer Scoop over Cargo for an install, but that's still potentially breaking it for some Windows users. Maybe I could commit the version that unpacks the result & repacks it, then submit a PR with this version for later when the MSRV is bumped?

That sounds like a good idea, I'll await it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants