Skip to content

(exa PR) 941: Show mounted filesystems and btrfs subvolumes#53

Closed
cafkafk wants to merge 13 commits into
mainfrom
pr-941
Closed

(exa PR) 941: Show mounted filesystems and btrfs subvolumes#53
cafkafk wants to merge 13 commits into
mainfrom
pr-941

Conversation

@cafkafk

@cafkafk cafkafk commented Jul 29, 2023

Copy link
Copy Markdown
Member

ogham/exa#941

tests
  • tests
  • optionflag
  • documentation
  • consider different output format

@cafkafk cafkafk changed the title (exa PR) 941 (exa PR) 941: Show mounted filesystems and btrfs subvolumes Jul 29, 2023
@cafkafk

cafkafk commented Jul 29, 2023

Copy link
Copy Markdown
Member Author

cargo test passes, this seems cool but perhaps it should be hidden behind a option flag?

@sbatial

sbatial commented Jul 29, 2023

Copy link
Copy Markdown
Collaborator

I like the idea as well.
I'd have to test it to give a final judgement.

But yes. I feel like this should be behind an option

Also: I don't see any new tests. It might make sense to add some for such a feature

@cafkafk

cafkafk commented Jul 30, 2023

Copy link
Copy Markdown
Member Author

Also: I don't see any new tests. It might make sense to add some for such a feature

Agreed, needs tests, option and documentation.

@sbatial sbatial mentioned this pull request Jul 30, 2023
63 tasks
@sbatial

sbatial commented Jul 30, 2023

Copy link
Copy Markdown
Collaborator

I have tested it and although I am a bit confused about the chosen rendering (the curly braces) it seems to work.

According to ogham/exa#434 (comment) @ogham found it very useful as well back then so looking good so far

@cafkafk

cafkafk commented Jul 30, 2023

Copy link
Copy Markdown
Member Author

although I am a bit confused about the chosen rendering (the curly braces)

Another thing to change before merging perhaps?

@cafkafk cafkafk added not ready for PRs that aren't finished and removed help wanted Extra attention is needed looking for testers labels Jul 30, 2023
@sbatial

sbatial commented Jul 30, 2023

Copy link
Copy Markdown
Collaborator

@daviessm seems to be active on github so maybe they have any insight on why they did it that way

@daviessm

Copy link
Copy Markdown
Contributor

This was the fist bit of Rust code I ever wrote, sorry if it's a bit strange. I'm open to changing any of the symbols used - I was awaiting feedback from the exa people. Particularly the flashing...

Regarding tests - is it possible to do tests without a btrfs filesystem?

I can add all of this in over the next few weeks if you give me some pointers.

@sbatial

sbatial commented Jul 30, 2023

Copy link
Copy Markdown
Collaborator

Regarding tests - is it possible to do tests without a btrfs filesystem?

I'm not sure but I would say it could be given that the information would have to be a file or file attribute.
I would need to look deeper into this myself but it looks like the crate used has some tests of their own.

Maybe they help to gain more insight: https://github.com/pop-os/proc-mounts/blob/master/src/mounts/mod.rs

@daviessm

Copy link
Copy Markdown
Contributor

Ok; should I start a new PR or should I submit a PR to this branch?

@cafkafk

cafkafk commented Jul 30, 2023

Copy link
Copy Markdown
Member Author

Ok; should I start a new PR or should I submit a PR to this branch?

I'd actually prefer a new PR (that way I can add myself as a reviewer), bonus points if you follow https://www.conventionalcommits.org/en/v1.0.0/

@cafkafk cafkafk added this to the exa pulls done milestone Jul 31, 2023
@sbatial

sbatial commented Jul 31, 2023

Copy link
Copy Markdown
Collaborator

Closing in favor of new PR.

@sbatial sbatial closed this Jul 31, 2023
@daviessm

Copy link
Copy Markdown
Contributor

New PR: #167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

not ready for PRs that aren't finished os › windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants