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

Support relative path imports from config file #7690

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

caojoshua
Copy link

Currently any relative path import will import relative from the alacritty executable. I believe its not useful. This PR makes the imports relative from the config file.

Nested relative imports work. For example, A can import B relative from A's path, and then B can import C relative from C's path.

One caveat is that alacritty migrate will expand relative paths to absolute paths. I noticed that this is the same behavior in that it expands ~/ (which was added in #4449).

Copy link
Member

@chrisduerr chrisduerr left a comment

Choose a reason for hiding this comment

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

Using relative imports at all seems like a poor choice. Why is this something we want to support?

@kchibisov
Copy link
Member

yeah, relative path are a bit confusing, especially when it comes to symbolic links, etc. paths relative to the home dir(which we support) are way more clear imho.

@caojoshua
Copy link
Author

Using relative imports at all seems like a poor choice. Why is this something we want to support?

Why do relative imports seem like a poor choice?

My use case is that in my configs, I have a git submodule for alacritty-theme. The configs are setup such that the relative paths are always consistent between alacritty.toml and the theme I want to import. Absolute paths are not guaranteed to be consistent across systems.

There are at least a handful of users who would like this support #4164 #5789

@kchibisov
Copy link
Member

it's not predictable with symlinks though, since you have 2 choices basically. And relative from home only breaks when you use windows and unix systems at the same time, I think, but the rest is always the ~/.config/alacritty/ or ~/. Though you could put your imports into the same place relative to HOME, which we already support.

@caojoshua
Copy link
Author

it's not predictable with symlinks though, since you have 2 choices basically.

I'm not understanding why this is an issue. The path will be relative from where Alacritty reads the alacritty.toml. I don't think there will ever be 2 choices. In general, programming and scripting languages allow for relative paths and I don't think they create extra confusion.

And relative from home only breaks when you use windows and unix systems at the same time

This does not apply to me, but this would help people under this use case

Though you could put your imports into the same place relative to HOME

Yes this is true. There are solutions without relative paths. I am suggesting supporting relative path imports as a quality of life feature.

@mxaddict
Copy link

I think relative paths are a good addition (Assuming that it's relative to the main config file)

Would allow for configs like so:

import = [
    "./tokyo-night.toml",
    "./common.toml",
]

Compared to what I currently have which is:

import = [
    "~/.config/alacritty/tokyo-night.toml",
    "~/.config/alacritty/common.toml",
]

@melMass
Copy link

melMass commented Mar 12, 2024

And relative from home only breaks when you use windows and unix systems at the same time, I think, but the rest is always the ~/.config/alacritty/

That's exactly why I need relative paths personally, I share common dotfiles for Windows native, wsl and macos.

My solution is indeed symlinking on all os to ~/alacrity-themes while storing this folder next to my config

Copy link
Member

@chrisduerr chrisduerr left a comment

Choose a reason for hiding this comment

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

I've contemplated this for a bit and I do think it makes sense to add, though there is some risk for breakage.

This PR uses the unresolved path if the configuration file is a symlink, which I think is the only thing that really makes sense. If people have a symlinked config, they should also symlink the imports.

I've considered if an alternative implementation would be changing Alacritty's working directory, but that would just break things and conflict with the working directory config option. We could set it just for subprocesses spawned by bindings, but I don't think there's any actual reason for why we'd bother.

Comment on lines +323 to +324
if let Some(config_path) = base_path.parent() {
path = config_path.join(path)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let Some(config_path) = base_path.parent() {
path = config_path.join(path)
if let Some(base_path) = base_path.parent() {
path = base_path.join(path)

Better to keep consistent naming to avoid confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants