feat: Variables history #977
Conversation
|
Thanks for opening this pull request! |
|
Hi @soar , I find it great that we could remember the values typed but It shouldn't be enabled only by setting an environment variable. This feature, if accepted, will need to be documented, so it will have to wait for #943 to be merged. I think we should make it visible that it's from the history and not a pre-defined value list, we need something to clearly differentiate both. Because variables with pre-defined values won't require to have an history of entered values. Does it impact in some way the scripting capabilities? Have you tested it? |
|
Hi, @alexis-opolka This feature works only for empty values, where no auto-suggestion commands are defined. And it makes sense: if you have a command to generate fresh and right values each time (considering dependencies) the history is not needed. I did this only for variables that are empty, because I found it frustrating that something needs to be typed each time, when you use a snippet. Like a URL for curl in the example. Or Docker images to pull. Or command to run in a pod. Something what is long enough to remember and type each time. We can rename the variable, of course. Thank you. |
src/env_var.rs
Outdated
| pub const CONFIG: &str = "NAVI_CONFIG"; | ||
| pub const CONFIG_YAML: &str = "NAVI_CONFIG_YAML"; | ||
|
|
||
| pub const VARIABLES_HISTORY_FILE: &str = "NAVI_VARIABLES_HISTORY_FILE"; |
There was a problem hiding this comment.
As said before, the name would need to change.
src/filesystem.rs
Outdated
| } | ||
|
|
||
| pub fn variables_history_pathbuf() -> Option<PathBuf> { | ||
| if let Ok(v) = env::var(env_var::VARIABLES_HISTORY_FILE) { |
There was a problem hiding this comment.
I think we should use option_env! instead of env::var and use the environment variable to take precedence on the configuration value but we should always fallback to the configuration value.
This means that you need to make an entry inside the configuration.
| let pathbuf = PathBuf::from(v); | ||
|
|
||
| if !pathbuf.exists() { | ||
| File::create(&pathbuf).unwrap_or_else(|_| panic!("Unable to create file: {}", pathbuf.display())); |
There was a problem hiding this comment.
I'm not sure about the use of the panic! macro here.
We can just not save the entry to the filesystem and output an error message afterward.
Navi doesn't rely on the variables history to work.
There is also the fact that we create the file inside the function that should just send back the path.
We should migrate this part to the save function.
| } | ||
| } | ||
|
|
||
| pub fn variables_history_pathbuf() -> Option<PathBuf> { |
There was a problem hiding this comment.
I would prefer the function to have the default prefix because the logic is the same as the other default functions.
|
@alexis-opolka As I mentioned, this PR is controversy and mainly a PoC. For me this is extremely useful and I use this already, but in this format it may be not for everyone. For example, it will save history for all inputs, even secrets. Or, if the same variable appears in different snippets, it will be shown everywhere. So, what I wanted is to make this setting totally optional, disabled by default, and to work only when the variable is set on the target system. If there is more than 1 person who will use this, we can re-iterate and add more settings. What you propose - using env variables at the compilation time, make an entry in the config, and rename the method to be |
|
Hi @soar , Since you've opened a PR in this repository I need to assume that there won't only be one user making use of this feature. I do think it would be best to have to compile Navi with an environment variable set to enable an experimental feature and for the ease of use and experiments have an entry inside the configuration file instead of relying on an environment variable during run time. As you said it yourself, this feature is currently doing a catch-all with no distinction of whatever you typed in which is not something that can be enabled by default and I completely agree on that. However being experimental or not does not make this feature exempt of being documented for future users, contributors or even the maintainers. In the current state of this PR, please understand that I cannot agree on merging it onto the main branch. |
|
The It is used for the cheats path which has a default path but can be modified by the users to point to another path.
I hope this solves your confusion on this subject. |
|
@alexis-opolka I still feel a misunderstanding here. Do we both agree, that if I change |
No @soar, what I mean by using See this function for the default cheats path: Lines 55 to 67 in 138b7e0 We are using So take for example
I want this feature to work in the same way. If I might repeat myself but WE DO NOT enable this feature by default. The logic is the same as the one you currently have except we don't rely on an environment variable while running but during the compilation. |
|
Maybe some code will help you understand what I mean: pub fn default_variables_history_pathbuf() -> Result<PathBuf> {
let mut pathbuf = get_data_dir_by_platform()?;
pathbuf.push("navi");
pathbuf.push("var.history");
Ok(pathbuf)
}
pub fn get_variable_history(variable: &str) -> Vec<String> {
// If the feature isn't enabled, exit immediately
if ! is_var_history_enabled() {
return vec![]
}
let pathbuf = default_variables_history_pathbuf().unwrap();
let file = match File::open(pathbuf) {
Ok(file) => file,
Err(_) => return vec![],
};
let reader = BufReader::new(file);
reader
.lines()
.map_while(|line| line.ok())
.filter_map(|line| {
let parts: Vec<&str> = line.splitn(2, ':').collect();
if parts.len() == 2 && parts[0] == variable {
Some(parts[1].to_string())
} else {
None
}
})
.collect()
}
pub fn save_variable_history(variable: &str, value: &str) {
// If the feature isn't enabled, exit immediatly
if ! is_var_history_enabled() {
return;
}
let pathbuf = default_variables_history_pathbuf().unwrap();
// Create the file if it doesn't exist
if !pathbuf.exists() {
File::create(&pathbuf).unwrap();
}
let mut file = std::fs::OpenOptions::new()
.append(true)
.create(true)
.open(pathbuf)
.expect("Unable to open history file");
writeln!(file, "{}:{}", variable, value).expect("Unable to write to history file");
}and pub fn is_var_history_enabled() -> bool {
if let Some(history) = option_env!("NAVI_VAR_HISTORY") {
if history == "true" {
return true
}
}
false
}This way, the feature is enabled only if we set the variable during the compilation of navi. EDIT: The code is available on my fork at the following address: https://github.com/alexis-opolka/navi/tree/var-history |
Signed-off-by: alexis-opolka <[email protected]>
|
I think this is the point of misunderstanding:
If you check my demo, the idea of this variable was to give USERS power to decide if they need history or not.
In the demo recording, you can see, that I set this variable in USER space as the first step. This is the main purpose of env variables, to be able to set features based on the environment. Using What I tried to do is more similar to this logic: Line 18 in 138b7e0 I'm sorry if I explained my idea wrong. |
|
Hi @soar , I think this is indeed the point of misunderstanding, I might not have explained well and I'm sorry for that. In the line you gave me, the EnvConfig struct is using a userspace variable during the execution but those entries in the configuration are not only present in EnvConfig but in YamlConfig, Config and maybe in ClapConfig. For an experimental feature, I do not want to rely on only one way to enable it and surely not a userspace environment variable without having an entry whatsoever in one of the Config structs. I need to have a defined way to know if the feature is enabled or not, be it for debugging purposes, more experimentation, whatsoever the goal afterwards. This is the main reason of why I asked you to put an entry in the configuration and being in the configuration does not mean being enabled by default. By having an entry in the configuration, say in That is why I proposed you, if you wanted to enable the feature with an environment variable and only that to instead use the environment variable during compilation. Once again, I understand the risks tied to this feature in its current state and I do not want this feature to be enabled by default, I want this feature to be disabled as a default just like you. I hope this solves a part or all of the confusion between us, if this is not the case do not hesitate to ask me more questions. |
|
I'm running the CI in the meanwhile since I already reviewed your code. |
|
@soar If you need me to show you via a code snippet what I mean don't hesitate to ask! 😄 |
|
Hi @soar, |
|
Sorry, I didn't have time to check it. But I believe we can close it, as it doesn't make sense to merge the code, if we need to rebuild the app to use it. I can keep it in my fork for now, maybe I will improve it later. |
|
Hi, I would still like to keep your code for later since I agree that this is a great feature. That way we will have a trace left for future work on that feature. |
|
Sure. Created: #981 |
2 participants
what
If env variable
NAVI_VARIABLES_HISTORY_FILEis set, stores information about entered variables, so they will be suggested next time the snippet is selected.This is PoC, but in the future it can be expanded:
demo