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

Add support for --literal flag and literal entry in config #900

Merged
merged 7 commits into from
Sep 19, 2023

Conversation

PanGan21
Copy link
Contributor

@PanGan21 PanGan21 commented Sep 11, 2023

Add support for --literal in order to opt out showing filenames with quotes
Closes: #894


TODO

  • Use cargo fmt
  • Add necessary tests
  • Add changelog entry
  • Update default config/theme in README (if applicable)
  • Update man page at lsd/doc/lsd.md (if applicable)

@muniu-bot
Copy link

muniu-bot bot commented Sep 11, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: PanGan21

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@zwpaper zwpaper left a comment

Choose a reason for hiding this comment

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

thanks for the contribution!

it mostly looks great! only with some nit we can discuss

doc/lsd.md Outdated
@@ -140,6 +140,9 @@ lsd is a ls command with a lot of pretty colours and some other stuff to enrich
`--header`
: Display block headers

`--literal`
Copy link
Member

Choose a reason for hiding this comment

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

according to https://man7.org/linux/man-pages/man1/ls.1.html

we can add -N as short name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed now!

doc/lsd.md Outdated
@@ -140,6 +140,9 @@ lsd is a ls command with a lot of pretty colours and some other stuff to enrich
`--header`
: Display block headers

`--literal`
: Do not show quotes on filenames
Copy link
Member

Choose a reason for hiding this comment

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

print entry names without quoting

ref: https://man7.org/linux/man-pages/man1/ls.1.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it as well according to the manual

src/app.rs Outdated
Comment on lines 184 to 186
/// When showing files do not quote their names
#[arg(long)]
pub literal: bool,
Copy link
Member

Choose a reason for hiding this comment

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

-N and description align with https://man7.org/linux/man-pages/man1/ls.1.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it!

@@ -71,7 +73,7 @@ impl Core {
// or require a raw output (like the `wc` command).
inner_flags.layout = Layout::OneLine;

flags.should_quote = false;
Copy link
Member

Choose a reason for hiding this comment

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

how about changing should_quote to literal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed!

@zwpaper zwpaper added this to the v1.1.0 milestone Sep 14, 2023
src/meta/name.rs Outdated
@@ -153,21 +153,21 @@ impl Name {
format!(
"{}{}",
icons.get(self),
self.hyperlink(self.escape(self.file_name(), quote), hyperlink)
self.hyperlink(self.escape(self.file_name(), !quote), hyperlink)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you think quote arg should change to literal as well in order to avoid this negation here

Copy link
Member

Choose a reason for hiding this comment

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

Yes! it would be great if you can also update this one!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it as well!

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2023

Codecov Report

Merging #900 (ff7b194) into master (93b3fb0) will decrease coverage by 0.46%.
The diff coverage is 92.10%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #900      +/-   ##
==========================================
- Coverage   85.84%   85.38%   -0.46%     
==========================================
  Files          50       51       +1     
  Lines        4932     4996      +64     
==========================================
+ Hits         4234     4266      +32     
- Misses        698      730      +32     
Files Changed Coverage Δ
src/app.rs 33.33% <ø> (ø)
src/core.rs 0.00% <0.00%> (ø)
src/meta/name.rs 87.07% <71.42%> (-0.26%) ⬇️
src/config_file.rs 72.72% <100.00%> (+0.27%) ⬆️
src/display.rs 89.84% <100.00%> (+6.61%) ⬆️
src/flags.rs 100.00% <100.00%> (+3.12%) ⬆️
src/flags/literal.rs 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zwpaper
Copy link
Member

zwpaper commented Sep 17, 2023

hi @PanGan21, can you fix the CI errors

@PanGan21
Copy link
Contributor Author

hi @PanGan21, can you fix the CI errors

I was using wrong rustc version, so I had to update Cargo.lock. Now I reverted it and used the correct rust version.
Could you run the ci one more time to check if everything is fixed now?

@zwpaper zwpaper merged commit bfbb217 into lsd-rs:master Sep 19, 2023
18 checks passed
@zwpaper
Copy link
Member

zwpaper commented Sep 19, 2023

thanks for the contribution! @PanGan21

@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [v1.0.0] - 2023-08-25

### Added
- Add support for `--literal` from [PanGan21](https://github.com/PanGan21)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have gone to an already-released v1.0.0?

Copy link
Member

Choose a reason for hiding this comment

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

good catch! actually, I was planning to use the GitHub auto-generate release note, this file may not be necessary, can you help create a PR to fix it and add a notice that we are no longer using this file, we use the GitHub auth-generate one instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not exactly sure what you meant, but opened one here: #907

Let me know if it is what you meant.

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.

Unable to disable quotes (no support for: --quoting-style, QUOTING_STYLE, or --literal)
4 participants