-
Notifications
You must be signed in to change notification settings - Fork 360
feat: add new fields for the ruler file
#2329
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
feat: add new fields for the ruler file
#2329
Conversation
|
Hello @SPFabGerman 👋
Even though being related to each other, I'd prefer having separate PRs for
The
I case you didn't know, you can simply use environment variables inside the {{env "PWD"}}
{{env "USER"}}
{{env "hostname"}}
The
The custom info is actually the first big thing I contributed to That being said, you don't need a {{with $.UserOptions.ref}} {{printf "(%s)" .}}{{end -}}I set the variable inside my cmd on-cd &{{
ref=""
if git rev-parse --is-inside-work-tree >/dev/null 2>&1; then
ref=$(git rev-parse --abbrev-ref HEAD 2>/dev/null)
if [ "$ref" = "HEAD" ]; then
ref=$(git rev-parse --short HEAD 2>/dev/null)
fi
fi
lf -remote "send $id :set user_ref '$ref'"
}}And here you can see it in action :D
To be honest I really don't see a point in introducing
We try to avoid breaking user configs by removing settings like this. Also, we already only calculate/populate the property when
Let's compare these two approaches with both also trying to show
I'd prefer working on this myself, I hope that is ok. The only real question I have to figure out is how truncation should work here, but that is another story... Also, I really don't think there is any value whatsoever in being able to sort by
Again, I don't see many people actually using these for sorting. Neither
The reason being that it is only ever used in the context of displaying a string. It is never used as a number. To wrap it all up, I honestly don't see a real use case for some of the changes while others are either already possible or need to be discussed. Regarding your other changes, I am open for discussion and perhaps @joelim-work has completely different opinions here. |
|
Hi, thanks for your feedback @CatsDeservePets. Other than that, I agree that allowing to set the |
You can either close this PR and create a new one or just revert the other commits (they won't appear in
Yeah, I think so too but that should be done in yet another PR. |
|
I largely agree with what @CatsDeservePets has said above. I will just add a few notes below:
|
8585214 to
d53438e
Compare
d53438e to
89441ad
Compare
|
@CatsDeservePets As discussed I limited this PR to the first commit, which only adds the new fields to the Rulerfile. I also added a second commit thats attempting to combine the existing
Personally I think option 3 or 4 is best, especially to avoid breaking user configurations. One final thing to note is that with this change the Size information for directories changes, which could technically be considered a breaking change. Previously directories were always reported with a constant size (4096 on my system). This is identical to what |
I guess I am fine with that. I agree that custom information per file are much simpler using
I am really unsure about this one. Let's quickly compare it to the way the if f.IsDir() && getDirCounts(d.path) {
switch {
case f.dirCount < -1:
info.WriteString(" !")
case f.dirCount < 0:
info.WriteString(" ?")
case f.dirCount < 10000:
fmt.Fprintf(&info, " %5d", f.dirCount)
default:
info.WriteString(" 9999+")
}
continue
}
var sz string
if f.IsDir() && f.dirSize < 0 {
sz = "-"
} else {
sz = humanize(uint64(f.TotalSize()))
}
fmt.Fprintf(&info, " %5s", sz)The biggest difference being that the I think there is no way around changing Other options are just leaving everything size related as it is or introduce another field. I don't have a good solution here. |
|
I don't think creating a wrapper for |
|
Honestly I'd say we just don't care about the size right now and perhaps add a ˋDircountˋ string property on user demand only. |
|
As suggested I removed the I also added a small wrapper for the |
|
Hello again, @SPFabGerman! Sorry for all the back and forth here. I'd prefer not to include the changes to the Thanks for your patience and work so far. Btw, if you prefer displaying {{.Size | humanize | printf " %5s" -}}with this {{if eq (substr .Permissions 0 1) "d" -}}
{{"-" | printf " %5s" -}}
{{else -}}
{{.Size | humanize | printf " %5s" -}}
{{end -}}You could also use |
5f032ba to
10f0060
Compare
Sounds good. I will open a new PR for the
I know and I do this already in my own config. But I would also really like to see the directory size in the ruler, which is partly why I created this PR in the first place. |
CatsDeservePets
left a comment
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.
Looks good. Do you want to update the docs as described in https://github.com/gokcehan/lf/blob/master/CONTRIBUTING.md as well as the CHANGELOG.md?
I can also do this myself after merging.
|
I added the documentation and changelog. |
5dbdf66 to
32bcda1
Compare
ruler file
|
Thanks once again. Looking forward to a separate issue regarding the |
| - Newline characters are now ignored when drawing the ruler with the `ruler` file configured (#2319). | ||
| - A potential crash when using the `scroll-up`/`scroll-down` commands is now fixed (#2320). | ||
| - Case-insensitive command-line completions no longer cause user input to be displayed in lowercase (#2336). | ||
| - The `ruler` file now supports `.Stat.Extension`, `.Stat.AccessTime`, `.Stat.BirthTime`, `.Stat.ChangeTime` and `.Stat.CustomInfo` (#2329). |
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.
This doesn't sound like a bug fix, should this be in the Added section of the changelog?
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.
Obviously! I did not even notice!
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.
Fixed in #2346



At the moment there is a weird discrepancy which information can be displayed where. Some information can only be shown in the info column (with the
set infocommand) and others only in the status / ruler line below (withset statfmtor the rulerfile),This PR aims to make all information consistently available everywhere, so that users can display information where they want to. (Coincedentally this also fixes #2325.) And as a little bonus I also added some
sortbyoptions for the new information.In detail I added the following:
infooption:lcount(link count),target(link target) andrsize(real size)sortbyoption:user,group,lcount,target,target-natural,rsizeExtension,AccessTime,BirthTime,ChangeTime,DirCount,TotalSize,CustomInfostatfmt:%f(name),%F(path),%e(extension),%z/%Z(total size),%A/%B/%C([abc]time),%i(custom info)Additionally I also added
Directory,UserandHostto the Rulerfile field to match whatpromptfmtprovides. These may come in handy for some users (for example asifconditions), especially as there are no ways to customize, reload or change a Rulerfile during runtime. (Similarly, the newStat.Extensioncan for example be used to provide colors for file names.)The
CustomInfofield is especially noteworthy as it allows users to customize the entirety of the rule line via shell scripts, far exceeding even what the current Rulerfile offers.However, there are some things that should be noted and improved before merging, but I wanted to first gather feedback and opinions:
rsizeoption andSizefield are pretty useless if compared tosize/TotalSize, as they only differ for directories and give each directory the same size. So they offer no real advantages over the existingsizeoption / newTotalSizefield.sizeinfo-option / sortby-option into two, one fordircountsand one for total dir size (this is for example already done instatfmtand the Rulerfile) and get rid of thedircountsconfiguration option entirely. Then we would only calculate thedircountson demand, so when the option is actually used ininfo,sortby,statfmtor the Rulerfile.targetinformation I also added the->arrows, as was suggested by [Feature Request] Symlink reference #2325. I think they look nice.targetinformation is prone to being entirely hidden when it's too long. I'm not sure if that is the intended effect or if it would be better to truncate it.user/groupsortby-options it might be better to sort by user / group id instead of name.linkCount()function to get link counts. But this function returns strings instead of integers (for some reason unbeknownst to me). This causes issues with sorting (though these could probably be resolved by using natural sort instead) and also means we can't make any integer comparisons with it in the Rulerfile.targetsortby-options I opted to order all symlinks first, instead of last. It seems natural to me that when a user wants to sort by symlinks they are probably mainly interested in symlinks, so they should appear first.TotalSizeRulerfile field andstatfmtpattern it might be better to provide explicit failure codes (similar to theDirCountfield) instead of having a 0-fallback.sortbyoption for Permissions, as there is no single good way to sort them.Documentation is still missing, but I can add that when all these things are resolved and finalized.