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

Fix :log-open when --log is specified #7573

Merged
merged 2 commits into from
Jul 9, 2023

Conversation

alevinval
Copy link
Contributor

The command always opened the default log file path. The path can be customized passing the --log optional argument. When this is the case the open-log command should respect it.

@alevinval alevinval changed the title fix/commands: the command open-log should open the correct file fix: command open-log should open the correct file Jul 8, 2023
pascalkuthe
pascalkuthe previously approved these changes Jul 8, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

thanks!

@pascalkuthe pascalkuthe added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer. A-command Area: Commands labels Jul 8, 2023
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I think we should use the same strategy as the config file so we don't need to tote around this log_file parameter on more structs and constructors:

static CONFIG_FILE: once_cell::sync::OnceCell<PathBuf> = once_cell::sync::OnceCell::new();
pub fn initialize_config_file(specified_file: Option<PathBuf>) {
let config_file = specified_file.unwrap_or_else(|| {
let config_dir = config_dir();
if !config_dir.exists() {
std::fs::create_dir_all(&config_dir).ok();
}
config_dir.join("config.toml")
});
// We should only initialize this value once.
CONFIG_FILE.set(config_file).ok();
}

pub fn config_file() -> PathBuf {
CONFIG_FILE
.get()
.map(|path| path.to_path_buf())
.unwrap_or_else(|| config_dir().join("config.toml"))
}

Log file can be customized with `--log` argument, the `open-log` command
should respect that.

As part of this work, also done some boy-scouting around initialisation
of directories, cleaning up and making sure they happen in the same place.
@alevinval
Copy link
Contributor Author

@the-mikedavis really nice! much cleaner, I've also done some boy-scouting around that code, let me know if you are cool with it.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I believe we also need an ensure_log_dir since that may be in a different directory than config (by deafult it is for example). I think it would be fine for those functions to be called in the initialize_ functions since those are only meant to be called once at initialization anyways

Also, updated the getters to `unwrap()` as it seems silly to default again
in case the cell is not initialised. If the cell is not initialised
it would be a big programming error, as the first thing we do when starting
the editor is precisely that.
Comment on lines 123 to +128
pub fn config_file() -> PathBuf {
CONFIG_FILE
.get()
.map(|path| path.to_path_buf())
.unwrap_or_else(|| config_dir().join("config.toml"))
CONFIG_FILE.get().map(|path| path.to_path_buf()).unwrap()
}

pub fn log_file() -> PathBuf {
LOG_FILE.get().map(|path| path.to_path_buf()).unwrap()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was about to write the following here (and the equivalent for the config_file())

unwrap_or_else(|| {
   initialize_log_file(None)
   log_file()
})

But then I really think we should unwrap() and potentially fail. A failure here means we have seriously broken the initialization steps in the main.rs, it seems less misleading than re-replicating the initialization of the cell in two places.

@the-mikedavis the-mikedavis changed the title fix: command open-log should open the correct file Fix :log-open when --log is specified Jul 9, 2023
@pascalkuthe pascalkuthe merged commit 1698992 into helix-editor:master Jul 9, 2023
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants