-
-
Notifications
You must be signed in to change notification settings - Fork 442
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
Git integration #822
Git integration #822
Conversation
this PR follows a discussion in #7 (comment) |
The only unsupported target is |
Updated with master |
803f5db
to
2826949
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done some of the reviews, and allow me to leave the rest to a next time.
I have checked the simple part, it only has some nit requests to change, and I am trying to walk through the complicated parts, trying my best to get this merged
Thx @zwpaper for the review. I'm sorry for the format issues; my conclusion: don't blindly trust code formatting |
@zwpaper is everything ok for you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @hpwxf, sorry again for the late reply, I was swamped recently, I had added more comments and it would be ok to go after we resolve them.
thanks so much for the PR, it looks great and we only have some nit to discuss.
let's make the magic happen!
hi @hpwxf, do you have some cycle for this PR, we are planning for 1.0 release, we can try to make it ship with the 1.0 |
Sorry for my discontinued availability. I will do the requested changes today or tomorrow. |
Thx for the review. Everything solved (see comments for details) |
hi @hpwxf, thanks for the great work again, I have only two more nit questions
Maybe a more consistent choice for the default would be better. how about a dash |
@zwpaper Thank you for your feedback. |
hi @hpwxf, I would prefer icons, or Unicode, actually, but as you said that it may be too close in some cases, that would be a problem, and it would be too far away if a space added🤣 how about we use ASCII in this first version, and try to figure out a theme that fits all cases? and we can also let users create a theme file to custom it themselves later. |
Hi @zwpaper ;
I will work on that. (I will first update to only use ASCII chars and then about the theme; if you prefer the git theme can be done in a next PR). |
Git theme done: In config file, you can define: # == Git ==
git:
# Whether to display git status
# Possible values: false, true
enabled: true
# How to display git status
# When "classic" is set, this is set to "default".
# Possible values: default, <theme-file-name>
# when specifying <theme-file-name>, lsd will look up theme file
# XDG Base Directory if relative, e.g. ~/.config/lsd/themes/<theme-file-name>.yaml,
# The file path if absolute
theme: git and default: "-"
unmodified: "."
new-in-index:
new-in-workdir: "?"
deleted: D
modified: ✍
renamed: R
ignored: I
typechange: T
conflicted: C I still have a small bug that always enable git when |
Hi, @hpwxf thanks so much for implementing the theme in this short time! But I prefer a separate PR for the theme features. One reason is that we would have more discussions about it like, we were planning to use a fixed theme file name, theme should be optional so that user doesn't have to offer all items, eg., keeping it simple would let us release the 1.0 in a shorter time. |
Ps it would also be great if we make the git theme (in a separate PR) in time to release with the 1.0 |
git feature is disabled on non-supported targets or using no-git feature.
760a4e8
to
9c7476d
Compare
Hi @zwpaper ; all done.
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #822 +/- ##
==========================================
- Coverage 87.08% 86.48% -0.60%
==========================================
Files 45 49 +4
Lines 4258 4602 +344
==========================================
+ Hits 3708 3980 +272
- Misses 550 622 +72
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@hpwxf any news when the git-theme branch will be merged? Is there an issue which tracks the announced discussion around the git-theme? Or is it already possible defining custom icons for the git-status column? |
Add git integration following a behaviour very close to what
exa
do.It adds a
--git
option and a blockgit
which is only display when--long
option is enable.git
is called only once for the current tree (if the current directory is a subdirectory of git, the status is displayed but if you use a deeper display, e.g. using--tree
, the git status of repositories in subdirectories is not done).cargo fmt