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

Improve config defaults #47

Merged
merged 4 commits into from
Apr 11, 2021
Merged

Improve config defaults #47

merged 4 commits into from
Apr 11, 2021

Conversation

sayanarijit
Copy link
Owner

@sayanarijit sayanarijit commented Apr 10, 2021

  • Rename custom field for node metadata to meta.
  • Move icon to meta.icon.
  • Rename normal_ui to default_ui.
  • Rename filetypes to node_types.
  • Split modes into modes.builtin and modes.custom.
  • Add the missing create file mode.
  • Rename focused_ui to focus_ui.
  • Make general.table.header non-nullable.
  • Add support for incremental configuration updates.
  • Add key binding ~ to go to homedir.
  • Add customizable cursor and prompts.
  • Improve the help menus.
  • Initial BDD testing setup
  • Improve version compatibility

Ref: #45

@sayanarijit
Copy link
Owner Author

Hey @maximbaz This PR should support the idea: what if users would only put in the config file what's different, not the entire config? . Can you please test it locally?

@maximbaz
Copy link
Contributor

Here's what I'm trying, doesn't seem to be working or it's not working as I was imagining 🙂

$ cat ~/.config/xplr/config.yml
version: v0.3.13

filetypes:
  directory:
    icon: "d"
  file:
    icon: "f"
  symlink:
    icon: "s"
    style:
      fg: Red

Does this work for you, give you different icons than standard?

@sayanarijit sayanarijit force-pushed the improve/config branch 2 times, most recently from eb68c7f to f14cf02 Compare April 10, 2021 17:17
@sayanarijit
Copy link
Owner Author

Actually filetypes has been renamed to node_types.

@sayanarijit
Copy link
Owner Author

Then as of now add_modifier and sub_modifier is needed. I'll be fixing it in a min.

@sayanarijit
Copy link
Owner Author

Done, you can try

version: v0.3.13

node_types:
  directory:
    meta:
      icon: "d"
  file:
    meta:
      icon: "f"
  symlink:
    meta:
      icon: "s"
    style:
      fg: Red

@sayanarijit
Copy link
Owner Author

@maximbaz If possible, also test the key bindings.

@sayanarijit
Copy link
Owner Author

Example:

modes:
  builtin:
    default:
      name: default
      key_bindings:
        on_key:
          ".":
            help: "foo action"
            messages:
              - LogInfo: foo

@maximbaz
Copy link
Contributor

Yeah, it works! And I think this approach is really cool!
I'll test some more, key bindings and other things 👍

I'd like to add to the topic of versioning configs:

  1. If I take the config above (the one with icons) and simply replace version with 0.3.12 (so one patch back), it all still works, but I'm getting a log message below - even though I dont have to change anything, my small icon override still perfectly works.
  2. If on the other hand I keep the version 0.3.13 and I use "wrong" config, e.g. filetypes instead of node_types, I don't get any warning or error, it just silently doesn't work.

I think it will be more ideal if we don't bug user if there is literally nothing for them to change, but we should definitely bug users if their config doesn't apply cleanly.

In other words, if something changed in the config, it's either change of default that I didn't override (in which case yay, I dont need to do anything), or it's a change in default that I did override anyway (in which case still I dont need to do anything, I already configured my preferred option), or it's a "breaking" change in which case my config doesn't apply anymore - in this case, I want to be notified 🙂

What do you think?

@sayanarijit
Copy link
Owner Author

sayanarijit commented Apr 10, 2021

The problem with that approach is detecting what the user has overwritten. Even though detecting changes in the config format might be somehow possible, it'll be impossible to detect changes in the commands. e.g.

BashExec: |
  - LogInfo: foo >> $XPLR_PIPE_MSG_IN

Users will never know if I change something in LogInfo or $XPLR_PIPE_MSG_IN.

@sayanarijit
Copy link
Owner Author

sayanarijit commented Apr 10, 2021

Also, this PR will be a major (comparatively) change. So, you need to update from 0.3.13 to 0.4.0. And you will get an error instead of the log. The app will never load unless you migrate your config.

@sayanarijit
Copy link
Owner Author

sayanarijit commented Apr 10, 2021

However... I think I can implement a logic something like

match config_version {
  "0.3.13" => Compatible,
  "0.3.13" => Compatible,
  _ => InCompatible,
}

And users will only see errors/log when they need to.

@maximbaz
Copy link
Contributor

Good example, and yes, we could try this!

By the way, have you considered following https://semver.org? I think many users would be familiar that if it's a change in major version, breaking changes are expected, while minor/patch version changes would indicate that their configs will work for sure?

@sayanarijit
Copy link
Owner Author

sayanarijit commented Apr 10, 2021

I'm following something similar but a somewhat simpler approach in my mind.
1.0.0 -> 1.0.x > bug fix (fully compatible).
1.0.0 -> 1.x.x > only backwards compatible. You can't use for e.g. app-1.0.0 with config-1.1.0. But vice versa is fine.
1.0.0 -> x.x.x > Not compatible at all.

However, until we're v1, I'm using the minor version as the major version and the bug fix version as the minor.
There is no bug fix version as of now.

@maximbaz
Copy link
Contributor

... and I use "wrong" config, e.g. filetypes instead of node_types, I don't get any warning or error, it just silently doesn't work.

Try this:

--- a/src/config.rs
+++ b/src/config.rs
@@ -474,6 +474,7 @@ impl ModesConfig {
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(deny_unknown_fields)]
 pub struct Config {
     #[serde(default = "default_config::version")]
     pub version: String,

It crashes in runtime, maybe we could make this into an error log instead? This would be very helpful I think...

@sayanarijit sayanarijit force-pushed the improve/config branch 2 times, most recently from 838d7da to 3066dcd Compare April 11, 2021 03:01
@sayanarijit
Copy link
Owner Author

sayanarijit commented Apr 11, 2021

Thanks to anyhow, we already have a great workable error message.

Error: unknown field `node_type`, expected one of `version`, `general`, `node_types`, `modes` at line 2 column 1

- Rename `custom` field for node metadata to `meta`.
- Move `icon` to `meta.icon`.
- Rename `normal_ui` to `default_ui`.
- Rename `filetypes` to `node_types`.
- Split `modes` into `modes.builtin` and `modes.custom`.
- Add the missing `create file` mode.
- Rename `focused_ui` to `focus_ui`.
- Make `general.table.header` non-nullable.
- Add support for incremental configuration updates.

Ref: #45
@sayanarijit sayanarijit force-pushed the improve/config branch 4 times, most recently from 49d1b34 to ce5cd3a Compare April 11, 2021 05:33
@sayanarijit
Copy link
Owner Author

Done, @maximbaz please review when you get time.

From this version, xplr won't annoy the users to visit the upgrade guide
when there is no need.

Also, users will only get upgrade related notification when it is
there is one.
@maximbaz
Copy link
Contributor

This is really awesome, I think it's a great improvement!

Regarding the error, at first I was thinking maybe preventing the app from starting is too harsh and we should let the app open (and simply ignore that unknown part of the config), and show error in the bottom part of xplr UI (as log message). But then I saw we prevent the startup when config version changed anyway, so maybe it's fine 🙂

Also,

- Add key binding `~` to go to homedir.
- Add customizable cursor and prompts.
- Improve the help menus.
@sayanarijit sayanarijit merged commit 055c108 into main Apr 11, 2021
@sayanarijit
Copy link
Owner Author

Done and released https://github.com/sayanarijit/xplr/wiki/Upgrade-Guide#v0313---v041.

@sayanarijit sayanarijit deleted the improve/config branch April 12, 2021 06:23
@maximbaz
Copy link
Contributor

Hey @sayanarijit, just wanted to follow-up on this, it generally works well but I found two cases to consider:

  1. It seems impossible to override prefix and suffix of *_ui options, unless I'm doing something wrong? (I simply want to add spaces on both sides, but any change doesn't seem to be reflected)
general:
  default_ui:
    prefix: "   "
    suffix: " "
  focus_ui:
    prefix: "▸[ "
    suffix: " ]"
    style:
      fg: Cyan
  selection_ui:
    prefix: " { "
    suffix: " }"
  1. It's probably impossible to override arrays, so e.g. I wanted to remove the third column "index", but I guess the only option for me would be to override entire header.cols and row.cols. I don't have a good alternative right away, so just curious if you have some good ideas 🤔

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