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

Refactor update-symlinks script #4

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

rjprins
Copy link

@rjprins rjprins commented Feb 9, 2021

  • Order functions to read top to bottom
  • Use small helpers functions for readability
  • Add some type hints

@wbolster wbolster self-assigned this Feb 9, 2021
@wbolster
Copy link
Owner

wbolster commented Feb 9, 2021

thanks 🙏 this ancient py2 compatible code could use some 💕 indeed...

i spotted a few things; will have a closer look later:

  • it crashes on this line in my symlinks.conf...

    .vim/ -> Vim/
    

    the problem seems to be the trailing slashes, and stripping those makes it work again. the slashes carry no meaning but serve as a ‘self-documenting’ note that this is about a directory. stripping them seems harmless.

  • resulting symlinks now point absolute paths instead of relative paths.
    before:

    .bashrc -> Configuration/Bash/bashrc
    

    after:

    .bashrc -> /home/wbolster/Configuration/Bash/bashrc
    

    it does not matter much (both work), but i'm curious whether this was accidental or intentional?

  • the type hints make this python3.6+ only. it may be a bit too new for some weird systems on which i use my dotfiles, e.g. synology ssh shells. i'll have to investigate further.

anyway, no action needed from you, these are mostly notes for myself 🙃

@wbolster wbolster marked this pull request as draft February 9, 2021 23:18
@rjprins
Copy link
Author

rjprins commented Feb 10, 2021

I changed the script years ago, so don't really remember exactly what I changed. I think the absolute path vs relative path was to symlink /etc/ files as well. I wanted to run the script from a different directory than the symlinks.conf file? Something along those lines.

Base automatically changed from master to main March 16, 2021 13:11
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