Rewrite set-theme to avoid file corruption

[Shai-Aviv]

This avoids file corruption (which necessitated calling sleep(1) ) by changing each file only once.
With these changes, set-theme --toggle-bg should toggle the background for the active theme.
These changes also reduces the memory usage, since only one line is read (and written) at a time.

[nickjj]

Hi,

Thanks a lot for the patch.

As for the new implementation, aren't we making a pretty big trade off here too? Now it scans the file line by line looking for matches to write and then it writes the changes as it finds the matches. I haven't used fileinput before. But the code looks like it might still be writing to the file multiple files? Does sys.stdout.write(line) perform the write at that point?

I also wonder if there's any readable patterns we can use to de-dupe the set up around using fileinput. Basically abstract the concept of using it to do inplace edits on a file and loop over each line. This way we only need to pass in what's being replaced.

[Shai-Aviv]

Hi Nick - thanks for considering my patch.

Yes, sys.stdout.write(line) performs the (buffered) write to the file. I still think it's a considerable improvement because the file is written then closed only once. As a result, if some process listens to an IN_CLOSE_WRITE event (or the like), that event will only trigger once. With the existing code, files are written and closed multiple times.

As for de-duping, we can define something like this:

def inplace_edit(file):
    return fileinput.input(files=(file,), inplace=True)

then use it like this:

with inplace_edit(MYFILE) as f:
    for line in f:
        process(line)

where process(line) writes to stdout using sys.stdout.write or just print.

[nickjj]

Oh I see.

So in the old case it's doing an open + close + [update content] + write + close and now it's just doing an open + close while 0 or more writes happen while it's open?

And the corruption happens with the old code because that close event is happening twice and it's too quick for the MS terminal to deal with it due to the way it's listening for changes? It most likely does watch that config file because updating the config will have its effects take place within a reasonable amount of time (100-200ms).

Are we still at a risk for corruption with the new code if the MS terminal happens to poll the file while it's open during the inplace_edit?

[Shai-Aviv]

There's no harm in doing: open (r mode) -> read contents -> close. I believe the problem with the existing code is that the following is done twice: open (w mode) -> write contents -> close.

The first write happens in this line:

dotfiles/.local/bin/set-theme

Line 199 in 42be51f

change_terminal_theme(args.theme)

and the second write happens in this line:

dotfiles/.local/bin/set-theme

Line 128 in 42be51f

change_terminal_theme(theme)

Here's a timeline of what I suspect is going on:

set-theme	Windows Terminal
Open settings.json (r mode)	Wait for settings.json to change
Read contents of settings.json
Close settings.json
Open settings.json (w mode)
Write contents of settings.json
Close settings.json	The file has changed (closed after writing). Start reading settings.json
Open settings.json (w mode)
Write contents of settings.json	Read corrupted data (modified while reading)
Close settings.json

---

Are we still at a risk for corruption with the new code if the MS terminal happens to poll the file while it's open during the inplace_edit?

I believe Windows Terminal will open the file only after a writer closes it. As long as it does that, there should no longer be a risk for corruption.

[nickjj]

Thanks for the break down, it makes perfect sense now.

Let's get this merged, but it may involve a few minor formatting changes, splitting up commits, etc..

I'll address these in a review on some of the lines over the next couple of days.

I know some low hanging fruit will be adding 1 space between defining a variable and executing a block of code below it. There's also making that inplace_edit change. I guess it might get too tricky using yield to include the for loop in the function too?

[nickjj]

You're making this too easy haha. Those are really good.

Btw do you have a TL;DR on store_true and how it differs from the old set up? I haven't pulled down your branch yet to try it out.

[Shai-Aviv]

Thanks 😄

action='store_true' is equivalent to action='store_const', const=True, default=False. It should behave just the same.

[nickjj]

One difference I noticed is:

New version: if you run set-theme with no arguments it reports no output with an exit code 0.
Old version: if you run set-theme with no arguments it throws a theme required error with an exit code 2.

Although this has kind of a nice side effect of being able to run set-theme --toggle-bg if you want to toggle the bg of the current theme without having to explicitly put the theme name in even if it doesn't change. I like that.

But I think maybe if there's no theme and no toggle-bg flag it should probably continue to throw an error. Hopefully this can be pulled off without complicating argparse too much.

Also, I'm going to leave this thing in a 100ms Bash loop for the next hour and see if it corrupts anything.

[Shai-Aviv]

Thanks for the observation. I added a check which prints an error (with exit code 2) when no arguments are given.

Another thing I noticed is that edit_inplace will replace symlinks. So after running set-theme, .vimrc (for example) is no longer symlinked to ~/dotfiles/.vimrc. I don't know if it was like that before. If we want to preserve symlinks, I suggest to modify edit_inplace like this:

@@ -83,5 +83,5 @@ VIM_CONFIG = f'{HOME}/.vimrc'
 
-def edit_inplace(file):
+def edit_inplace(file, preserve_symlinks=True):
+    if preserve_symlinks:
+        file = os.path.realpath(file)
     return fileinput.input(files=(file,), inplace=True)
 

[nickjj]

The original kept the symlink in tact. Good catch tho, I didn't check for that in your version but I did have similar logic with the old sed commands before this was a Python script.

Can you amend these commits:

Feel free to amend 57c2df1 with your proposed edit_inplace change but you'll need to import os too.

For be4e66e it might be worth changing the argparse condition to check the length of args to see if it's empty (or 1, whatever the case is for not including args). This way we don't have to hard code the arguments. What do you think?

In 48482bd I noticed you're using exit() instead of sys.exit(). Should we stick with using sys.exit()? The docs mention passing in a string will result in an exit code of 1 so we should be good to go.

[Shai-Aviv]

Right, passing a string to exit() will print the string to sys.stderr and call sys.exit(1), which is what we want. But we can call sys.stderr.write(errormsg) and sys.exit(1) explicitly if you prefer.

I amended 57c2df1 → 8f921e9 with my proposed edit_inplace to preserve symlinks.

I also pushed b932f9f to check the length of args instead of hard-coding the arguments.

[nickjj]

I think we can get by with just calling sys.exit("foo") based on the Python docs of:

and if any other object is printed to stderr and results in an exit code of 1. In particular, sys.exit("some error message") is a quick way to exit a program when an error occurs.

It does hint about writing to stderr but maybe just passing a string to sys.exit calls sys.strderr.write() for you?

[Shai-Aviv]

sys.exit() does indeed write to sys.stderr when passed a string:

$ python3 -c 'import sys; sys.exit("bye")' > /dev/null
bye

$ echo $?
1

I also prefer sys.exit() over exit(), so let's use that.

Edit: I amended 8925a1d → 21ef2f6

[nickjj]

Could we use this newfound knowledge of sys.exit() to amend 2d938dc by removing the print call and passing that error message directly to sys.exit? I'm ok with it returning 1 instead of 2.

Also, I'm not in a position to test this now but what does parser.print_usage() do? Could that be removed too?

[Shai-Aviv]

I called parser.print_usage(sys.stderr) and sys.exit(2) just to imitate the way argparse handles errors. See https://github.com/python/cpython/blob/15bd9efd01e44087664e78bf766865a6d2e06626/Lib/argparse.py#L2573

This makes the argument errors look more consistent:

$ set-theme --foo
usage: set-theme [-h] [--toggle-bg] [THEME]
set-theme: error: unrecognized arguments: --foo

$ set-theme
usage: set-theme [-h] [--toggle-bg] [THEME]
set-theme: error: at least one argument is required

parser.print_usage(sys.stderr) prints the line that starts with usage:

[nickjj]

Ok fair enough, let's keep it as is. Sometimes a bit of extra code is worth it to make the user experience better and more consistent.

One last thing, in b932f9f you have if not any(vars(args).values()). Could this be also written as something like: if len(sys.argv) == 1? A length of 0 wouldn't ever happen since I think the script name will always be the first item in the list (not in a spot to run a Python REPL atm).

[Shai-Aviv]

len(sys.argv) == 1 is definitely better, thanks. I decided to call ArgumentParser.error() directly - it is much cleaner:

ArgumentParser.error(message)
This method prints a usage message including the message to the standard error and terminates the program with a status code of 2.

https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.error

Edit: I also squashed some of the commits together to keep things organized

[nickjj]

Awesome thanks.

I'll pull this down and give it a run through a bit later today and then merge it in if all goes well (it should).

[Shai-Aviv]

I know my merge window is closing, but I had a great idea to make the argument parser more helpful (3692310):

$ set-theme foo
usage: set-theme [-h] [--toggle-bg] [THEME]
set-theme: error: argument THEME: invalid choice: 'foo' (choose from 'gruvbox', 'one')

[nickjj]

Oh wow, I didn't even know argparse did that out of the box.

There's always time for deleting code.

[Shai-Aviv]

With d518236, the themes are also listed automatically:

$ set-theme -h
usage: set-theme [-h] [--toggle-bg] [{gruvbox,one}]

Set a theme along with optionally toggling dark and light backgrounds.

positional arguments:
  {gruvbox,one}  the theme name

optional arguments:
  -h, --help     show this help message and exit
  --toggle-bg    toggle the background between dark and light

[Shai-Aviv]

About deleting code - I have to agree! I like to brag at work about my GitHub Enterprise stats - my deletions are roughly twice the amount of additions 😄

[nickjj]

This PR deletes more code than adding it, so you're winning there considering how much better the script is now.

Can you amend 3990482 by making the return and sys.exit lines be indented with 4 spaces instead of 2? It should pass running flake8 against it.

[Shai-Aviv]

I also dropped 189354a, as d518236 removes it anyway

[nickjj]

I think we're ready for a final test run / merge?

[nickjj]

Can you add an empty new line above the return statement to break it up from the if statement?

[nickjj]

Can you add an empty new line above L98 to break up the condition and the 2nd match variable?

[nickjj]

I've left a few minor recommendations on formatting.

Can you please amend the commits they are associated to?

[Shai-Aviv]

I amended the commits to fix the formatting as you suggested. Please give this a final test - I have nothing more to add (or remove 🙂)

[nickjj]

It's all good. Thanks a lot for the patch and for making the overall PR experience as good as it gets.

Edit:

I've updated the documentation to reflect this update since the steps to customize it for different terminals have changed.

I also updated my blog post to call out your PR at https://nickjanetakis.com/blog/a-terminal-tmux-vim-and-fzf-theme-switching-script-written-in-python#important-updates and pinned a comment in the YouTube video too https://www.youtube.com/watch?v=h509rn2xIyU.