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

Set location of state files to rational-config-var-directory #158

Merged
merged 1 commit into from
Jul 20, 2022
Merged

Conversation

telenieko
Copy link
Contributor

@telenieko telenieko commented May 18, 2022

Rational introduces the variable rational-config-var-directory which, apparently, aims to have Emacs save state files there.

This currently works for recentf and history, but there are a lot more files that are currently been kept inside emacs-user-dir.

Then,

  1. This can cause problems, like Ignore bookmarks file which shall never be commited #137, when contributing to this project as your git tree will be dirty.
  2. Introduces an inconsistency as some state files are in emacs-user-dir others are on rational-config-var-directory.

If the idea is to have state files in one place, there should be some way to do so.

This is a proposal:

  1. Use with-eval-after-load so the "new default" is only set if the package is loaded.
  2. customize-set-variableas early as possible to not override any user preferences
  3. Remove state files from .gitignore as they should no longer be on the tree.

I am placing all in rational-defaults as it looks right the right place:

  1. These are defaults
  2. Putting it in the appropiate module (ie: rational-org for the org-id-locations would not work if the user is not loading that module (but still expects state files to be in the right place.

BACKWARDS INCOMPATIBLE CHANGE: This PR makes no effort to move existing state files. Not sure if it should be attempted at all?

Keeping Draft status as I intend to rebase into a single commit after review.

@telenieko telenieko changed the title Set location of state files to rational-config-var-directory Draft: Set location of state files to rational-config-var-directory May 18, 2022
@telenieko telenieko marked this pull request as draft May 18, 2022 11:38
@telenieko telenieko changed the title Draft: Set location of state files to rational-config-var-directory Set location of state files to rational-config-var-directory May 18, 2022
@jeffbowman
Copy link
Contributor

Nice! Thanks for the PR. Some thoughts:

  • We might want to keep the .gitignore to avoid issues like Ignore bookmarks file which shall never be commited #137 for those who have used bookmarks or project files already. I'm not totally against removing them from .gitignore, just thinking about the fact that they might exist already and someone might miss the move to the rational-config-var-directory and end up accidentally committing something etc. At least leaving them in wouldn't change the status quo and does no real harm.
  • Can you also update the docs with this PR (see docs/CONTRIBUTING.org and docs/rational-defaults.org)

@jeffbowman
Copy link
Contributor

@telenieko Need any help with this? I'd love to get this merged!

@telenieko
Copy link
Contributor Author

@telenieko Need any help with this? I'd love to get this merged!

Slipped my mind 😅 will give a though about how to document this.

I am thinking some explaining regarding rational-config-var-directory should be added to rational-emacs.org and then rational-defaults.org can build on that explanation to explain what paths/settings are being modified.

I'd probably also split rational-defaults.org into "module settings" and "path changes" sections.

I'll draft something up during the week and push!

@telenieko
Copy link
Contributor Author

I am thinking some explaining regarding rational-config-var-directory should be added to rational-emacs.org and then rational-defaults.org can build on that explanation to explain what paths/settings are being modified.

Started working on this in #166, now on this PR I will:

I'd probably also split rational-defaults.org into "module settings" and "path changes" sections.

And go into more detail on how rational-defaults moves around configuration and runtime data into etc/ and var/

@jeffbowman
Copy link
Contributor

Looks like your changes to README.org and rational-emacs.org are on both PRs, this and #165. As it stands, they shouldn't conflict, but if you add more verbiage on the other PR there is the potential this will conflict. I appreciate your effort on this and I am looking forward to when it is ready to merge.

@jeffbowman
Copy link
Contributor

@telenieko Anything I can help with?

@telenieko
Copy link
Contributor Author

Hi !

@telenieko Anything I can help with?

I think manipulating spacetime so more hours fit in a day is not yet within Emacs powers! 🤣

Looks like your changes to README.org and rational-emacs.org are on both PRs (#165, #158) (...)

I was separating the general explanation from the rational-defaults specifics into two different PRs, though it is probably unnecessary and #165 may be simply killed.

I have just pushed some new code that fixes the documentation part, also makes the behavior optional (which is probably a good thing).

Last but not least, a long comment is now on rational-defaults.el explaining some possible drawbacks of how this is handled. For the current list of paths I don't see a problem but the warning is there for future consideration.

Let me know what you think of the new code

@jeffbowman
Copy link
Contributor

Code looks good. I wonder if it might fit better in the init.el just after the etc/ and var/ directories are created.

@telenieko
Copy link
Contributor Author

Code looks good. I wonder if it might fit better in the init.el just after the etc/ and var/ directories are created.

That would be the right approach but I think that would require:

  1. Some sort of "early config" system by which users can define configurations to apply before initialization. Which does not currently exist and would need some thinking first
  2. Some concept of "early modules" those being modules loaded during init instead of during config. Because what this PR introduces would be moved to some "rational-defaults-init" or something like that. Again, needs some thought.

I don't think either item has been discussed before? if not I'll try to give it some thought and open an issue to discuss it

@saccarosium
Copy link
Contributor

Hi @telenieko,
As discussed in #176, it would be nice adding a variable, something like rational-use-xdg-folders, that lets chose the user if he wants to use the var/etc directories or the xdg directories. This change, in my opinion, would make the project much more coherent in UNIX systems.

;; On Windows the xdg directories don't exists
(unless ON-WINDOWS
  (defvar rational-use-xdg-folders nil
    "Prefer XDG folders over rational-config-*-directory"))

(defvar rational-config-etc-directory (expand-file-name "etc/" rational-config-path)
  "The user's configuration etc/ folder.")
(defvar rational-config-var-directory (expand-file-name "var/" rational-config-path)
  "The user's configuration var/ folder.")

And then we can simply resign the etc and var variable to the xdg equivalent.

(when rational-use-xdg-folders
  (defvar xdg-data-home-directory
    (if (getenv "XDG_DATA_HOME")
      (getenv "XDG_DATA_HOME")
      (expand-file-name ".local/share/" (getenv "HOME"))))
  (defvar xdg-cache-home-directory
    (if (getenv "XDG_DATA_HOME")
      (getenv "XDG_DATA_HOME")
      (expand-file-name ".cache/" (getenv "HOME")))))

(unless rational-use-xdg-folders
  (defvar rational-config-etc-directory (expand-file-name "etc/" rational-config-path)
    "The user's configuration etc/ folder.")
  (defvar rational-config-var-directory (expand-file-name "var/" rational-config-path)
    "The user's configuration var/ folder."))

(when rational-use-xdg-folders
  (defvar rational-config-etc-directory (expand-file-name "emacs/" xdg-cache-home-directory)
    "The user's configuration etc/ folder.")
  (defvar rational-config-var-directory (expand-file-name "emacs/" xdg-data-home-directory)
    "The user's configuration var/ folder."))

@telenieko
Copy link
Contributor Author

Hi,

(...) #176, (...) rational-use-xdg-folders, that lets chose the user if he wants to use the var/etc directories or the xdg directories.

I left out XDG compliance out of this PR on purpose (more in a moment) mainly to keep it focused and with minimal disruptions.

My idea here has been to do the 1st step in that direction: have a place to "collect" all the variable names that need to be altered to move runtime & config data "to the right place". In reality, that is all this PR is doing: (defcustom rational-folders t ...

But this is step 1. Also, here I only touched rational-config-var-directory, but we also need to collect "runtime configurations" that should move to rational-config-etc-directory.

Moving forward there are several other things to do:

  1. Some sort of "early config" system by which users can define configurations to apply before initialization. Which does not currently exist and would need some thinking first.

This is needed because several of the paths being changed should be changed very early on (see comment in the code above the defcustom rational-folders).

  1. Proper folder best-practices compliance. This, on *NIX, means XDG. But on Windows there are other recommended paths for caches, etc. Also macOS has its own best practices.

This step would be achieved mainly by setting rational-config-etc-directory and rational-config-var-directory to a platform specific path, either under the new rational-folders option or a new one.

Both steps require some good thiking. Step 2 more so it needs to care about other supported platforms.


So, merging this PR enabled us to keep expanding the list of files "under control" by adding more and more entries like:

(with-eval-after-load 'project
  (rational-defaults--sensible-path rational-config-[var|etc]-directory
                                    'project-list-file
                                    "projects"))

While this advances, and more and more files are controlled we can open new PR about the other two issues. Otherwise this PR risks getting to broad in scope and take longer and longer to merge.


Let see if we can get this 1st step in and start thinking and working on the other two!

PS: I also believe step 2 (best practices compliance) should probably be an independent emacs package so the whole ecosystem can benefit of it. But this could always be done later.

@jeffbowman
Copy link
Contributor

Rational to crafted rename is complete, would like to get this merged when its ready. You mention a few things requiring deeper thinking above, are those show stoppers for you?

This aims to setup initial infrastructure to keep state files
inside `crafter-config-var-directory`.

- A setting is created to enable/disable the behaviour (enabled by default)
- A helper function is introduced to ease adding new state files
- This is all documented

Unrelated to the above is:

We add `CUSTOM_ID` to a documentation entry to enable creating an internal link.

This is because according to [orgmode documentation](https://orgmode.org/manual/Internal-Links.html) local
links by ID work only when `CUSTOM_ID` is set.

> (...) a construct like ‘[[#my-custom-id]]’  specifically targets the
entry with the ‘CUSTOM_ID’ property set to ‘my-custom-id’.

Attempting to export `README.org` without this results in an error.
@telenieko
Copy link
Contributor Author

telenieko commented Jul 20, 2022

Hi,

Rational to crafted rename is complete, would like to get this merged when its ready. You mention a few things requiring deeper thinking above, are those show stoppers for you?

Just pushed again, handled the renames; rebased & squashed the PR. Ready to merge

The other few things mentioned in the PR are things to consider for the future. See:

So, merging this PR enabled us to keep expanding the list of files "under control" by adding more and more entries like:
(...)
While this advances, and more and more files are controlled we can open new PR about the other two issues. Otherwise this PR risks getting to broad in scope and take longer and longer to merge.

@jeffbowman I could not test the changes now after the rename as I'm currently refactoring most of my Emacs config. Please check that it loads! (Though the code looks fine)

@telenieko telenieko marked this pull request as ready for review July 20, 2022 11:06
@jeffbowman jeffbowman merged commit d39cd6e into SystemCrafters:master Jul 20, 2022
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.

3 participants