Skip to content

Conversation

@Susensio
Copy link

Solves #10

Saúl Nogueras added 2 commits December 20, 2023 12:52
Prioritize XDG folder
Discards previous history
end
end

local function getHistoryFile(debugger, mode)
Copy link
Owner

@giann giann Dec 21, 2023

Choose a reason for hiding this comment

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

You could cache the response so we don't stat two files each time we add something to the history

Copy link
Author

Choose a reason for hiding this comment

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

I don't quite understand what you mean by that. Could you elaborate?

Copy link
Owner

Choose a reason for hiding this comment

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

You only need the information about which file is the right one once. So keep that information somewhere (cache it), and return that instead of checking files each time getHistoryFile is called.

Copy link
Author

@Susensio Susensio Dec 31, 2023

Choose a reason for hiding this comment

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

Where would you implement the cache? Function wise? Session wise on boot?
Implementing a cache is a delicate task that should be handled with care, with some edge cases.
I don't think checking for a file existence is a task worth caching, honestly.

In addition, is this duality between xdg files and home files a feature that you want to keep in croissant? I mean, is there a reason to keep it long term? I personally would migrate towards xdg completely, and not check for files in $HOME.
Maybe if you want to keep it customizable, a envvar could be read. I personally never use those per app config path envvars, I have $XDG_{FOLDER}_HOME if I ever want to move all my configurations

On the other hand, in my code for getHistoryFile I only stat one file (old $HOME/.croissanthistory is ignored.
Regarding the config file, the old config is only checked up if the xdg one does not exist.

@giann
Copy link
Owner

giann commented Dec 21, 2023

Thanks for this, see the only comment I had

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.

2 participants