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

create config for symlink arrow #436

Merged
merged 6 commits into from Nov 2, 2020
Merged

Conversation

zwpaper
Copy link
Member

@zwpaper zwpaper commented Oct 24, 2020

fix #409

@meain
Copy link
Member

meain commented Oct 27, 2020

The code part looks good. @kmoschcau do you have any other suggestion for the name of the group that will contain this(styles).
I am assuming in future we could add things like space between icon and name here as well.

@kmoschcau
Copy link
Contributor

@meain Yeah I don't think "styles" is a good way of naming it. At style to me is something like a set of settings, that you switch as a whole. It might be easier to comprehend, if you just keep this key to setting up icons/symbols and name it either "symbols" or "icons". That way you can reuse it for file type icons as well. Spacing can then be under its own key.

Also in terms of code organization, yes I understand to some degree that one might want to put this one setting into the symlinks module, but for future proofing it would be better to put it into its own icons (or similar) module.

@meain
Copy link
Member

meain commented Oct 28, 2020

I am also not sure if icons should be what this should go under, as that right now it has a relation to icons that are used for display with filenames. icons in itself would have 3 categories. general (ones for folder, generic file, and symlink), by_name, by_extension.

What I see here are mostly a bunch of unrelated configs (symlink-arrow, icon-spacing). What are you guys' opinion on dropping it at root level?

@zwpaper
Copy link
Member Author

zwpaper commented Oct 28, 2020

IMO, root-level options would make it simpler, but as the number of options grows, the config file may be a little bit messy, this is I would like to group it up like your previous comment, adding the number of space between icon and filename here.

styles may not be the best name, what I am trying to express is: this is a group of settings about the output styles of lsd

@zwpaper
Copy link
Member Author

zwpaper commented Oct 28, 2020

as code organization part, @kmoschcau you are right, I should have put it to a new mod.

when I first implement the symlink arrow, I treat it as an option for symlink, but later the styles option came up but I did not move the code.

If we reach an agreement, the styles(maybe a better name) module can be used to put those options.

@kmoschcau
Copy link
Contributor

@meain but why not group this together with the filename icons? To me this sound pretty sensible. Just to be clear, I would of course not put the spacing options in there as well. Those would go somewhere else.

@zwpaper after thinking some more about this, especially when putting options like spacing in this category, what about naming it "layout"?

@meain
Copy link
Member

meain commented Oct 28, 2020

@kmoschcau

Are you suggesting something like this? I am assuming we will need some classification for general, filename and filetype anyways. Or should symlink-arrow be under some other heading?

icons:
    symlink-arrow: <>
    general:
        - directory: <>
        - symlink: <>
        - broken-symlink: <>
        - file: <>
    filename:
        - Dockerfile: <>
        - node_modules: <>
    extension:
        - rs: <>
        - py: <>

@zwpaper
Copy link
Member Author

zwpaper commented Oct 28, 2020

The symlink arrow is a little bit special, it can be UTF-8 chars like the icon we talk about, but it does not serve like the icon in front of filenames indicating the file types.
That's why I am not adding it to the icon set but a styles, it's something like the space between icon and filename IMO.

But as you guys said, it also can be an icon sitting between the filename and target file, I am also ok with this, but prefer leaving the icon be the icon before filename

@meain
Copy link
Member

meain commented Oct 28, 2020

@zwpaper Yeah, that was what I was thinking too. icons would better be reserved for file icons. But not sure if styles is the best "group" for this though.

@zwpaper
Copy link
Member Author

zwpaper commented Oct 28, 2020

style is a little bit too general, but I can't figure out a better one.

If we can, I would also like to replace it.

layout fits the space option, but not the arrow one

@kmoschcau
Copy link
Contributor

kmoschcau commented Oct 28, 2020

How about a root key called "styling" and then have the "icons", "color-theme" (or similar), "layout" and all other miscellaneous keys like the "symlink-arrow" under it?

On second thought, that's pretty similar to the original proposal, but I think "styling" fits better than "styles".

@codecov-io
Copy link

codecov-io commented Oct 31, 2020

Codecov Report

Merging #436 into master will increase coverage by 2.02%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   78.54%   80.56%   +2.02%     
==========================================
  Files          34       35       +1     
  Lines        3360     3417      +57     
==========================================
+ Hits         2639     2753     +114     
+ Misses        721      664      -57     
Impacted Files Coverage Δ
src/display.rs 25.49% <0.00%> (ø)
src/flags/symlink_arrow.rs 88.23% <88.23%> (ø)
src/flags.rs 87.50% <100.00%> (+87.50%) ⬆️
src/meta/symlink.rs 69.81% <100.00%> (+63.35%) ⬆️
src/flags/sorting.rs 94.09% <0.00%> (+1.68%) ⬆️
src/flags/icons.rs 89.92% <0.00%> (+2.15%) ⬆️
src/flags/color.rs 91.56% <0.00%> (+2.40%) ⬆️
src/flags/ignore_globs.rs 41.17% <0.00%> (+7.35%) ⬆️
src/flags/recursion.rs 88.63% <0.00%> (+10.60%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ed0611...61296d1. Read the comment docs.

@zwpaper
Copy link
Member Author

zwpaper commented Oct 31, 2020

@meain changed according to discuss, good to go now

@meain
Copy link
Member

meain commented Oct 31, 2020

Hey @zwpaper , sorry that this taking a while. I have been super busy with work stuff recently. Just wanted to make sure we get the config right as that will stick forever.

Here is my argument for having this in the root than under a heading.
From what I can tell, pretty much everything else other that what we have now comes under styling, and when that is the only heading, it seems unnecessary. There are two options which we can take.
Either separate it out a bit more, also have the currently supported items under general or basic.
Another option would be to have most thing directly in root. We will sill have nested when it is needed, but things like this can go into the root.
Btw, since this is yaml, users can have comments if they wish to separate out parts of the config.

Just an example:

# general settings
layout: grid
color:
  when: always
icons:
  when: never

# display colors
color-theme:
  file: #fafafa
  directory: #fce3e3
  symlink: #dddddd
  borken-symlink: #ff0000

# custom icons
icons:
  filetype:
    rs: "<>"
    go: "<>"

# other settings
symlink-arrow: "->"
icon-spacing: 2

This to me actually looks better than nesting everything other than the top items in styling.
What do you think?

@zwpaper
Copy link
Member Author

zwpaper commented Oct 31, 2020

everything else other that what we have now comes under styling

this convinced me,looks like root level make sense too.

PS: never mind the delay 😁

@kmoschcau
Copy link
Contributor

@meain you are making a good point, I agree.

@zwpaper
Copy link
Member Author

zwpaper commented Nov 2, 2020

@meain should be ready to go now!

@meain meain merged commit 95535cb into lsd-rs:master Nov 2, 2020
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.

Feature request: allow symlink arrow to be "->" instead of "⇒"
4 participants