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 issue #3163: uutils: add dir and vdir utils #3405

Merged
merged 20 commits into from
Apr 15, 2022
Merged

fix issue #3163: uutils: add dir and vdir utils #3405

merged 20 commits into from
Apr 15, 2022

Conversation

gmnsii
Copy link
Contributor

@gmnsii gmnsii commented Apr 14, 2022

Fixes issue #3163 by adding the dir and vdir utils.

This is done by adding two function to ls: dir_main() and vdir_main(). Upon launching one of these utilities, they launch the corresponding functions. These function just add dir and vdir default arguments and then launch ls normally.

For example, in the vdir_main() function, we check if the user gave any formatting related argument and if he didn't, we add the -l argument to the config (as vdir is a shortcut to ls -l -b). This allows the user to overwrite dir and vdir default arguments. We then do the same thing for the other vdir default argument.

Since they use the same functions than ls I just added tests for the default output and for the output in case an argument is given.

@sylvestre
Copy link
Sponsor Contributor

Great work! Just a "small" comment

.help("Print help information.")
)
// Format arguments
.arg(
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

could you please refactor this ? it is a lot of duplicate content :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you please refactor this ? it is a lot of duplicate content :)

Done

Copy link
Sponsor Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

more nits

src/uu/ls/src/ls.rs Outdated Show resolved Hide resolved
.vscode/cspell.dictionaries/jargon.wordlist.txt Outdated Show resolved Hide resolved
src/uu/ls/src/ls.rs Outdated Show resolved Hide resolved
@gmnsii
Copy link
Contributor Author

gmnsii commented Apr 14, 2022

Committed suggestions

@sylvestre
Copy link
Sponsor Contributor

2022-04-14T20:08:28.5091941Z 'dir' was not built with uutils, using the 'false' program
2022-04-14T20:08:28.5146878Z 'vdir' was not built with uutils, using the 'false' program

I think you have to add them here too:
https://github.com/uutils/coreutils/blob/main/GNUmakefile#L54=

@gmnsii
Copy link
Contributor Author

gmnsii commented Apr 14, 2022

I added them

Copy link
Sponsor Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

Almost ready.
Could you please also remove vdir and dir from this line?

https://github.com/uutils/coreutils/blob/main/util/build-gnu.sh#L183=

chcon|dir|runcon|vdir) => chcon|runcon)

@@ -797,618 +797,713 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
list(locs, &config)
}

/// The entry point for the dir coreutils
pub fn dir_main(args: impl uucore::Args) -> UResult<()> {
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

sorry for one more comment
I am wondering if this is possible to move this function into dir.rs ?
to avoid ls.rs to be too big

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could but that would mean making a lot of refactoring in ls to make a lot of functions and structs accessible to dir and vdir. Notably the whole quoting style file that is a private ls module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could move it into uucore however

@gmnsii
Copy link
Contributor Author

gmnsii commented Apr 15, 2022

Done. Ignore what I said before, I forgot that pub mod was a thing

@sylvestre
Copy link
Sponsor Contributor

Looks great, i will merge if once the CI is green :)

@sylvestre sylvestre merged commit c2e214b into uutils:main Apr 15, 2022
@sylvestre
Copy link
Sponsor Contributor

Well done!

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.

None yet

2 participants