The configfile function isn't correct and the global function is used ambiguously as a local variable.

[Poikilos]

As noted on PR #1285 (but unrelated to it):

printrun/utils.py

def configfile(filename):
    '''
    Get the full path to filename by checking in the
    standard configuration directory (See the lookup_file
    function's documentation for behavior).
    '''
    return lookup_file(filename, [os.path.expanduser("~/.printrun/"), ])

Silly question maybe and not directly related to this PR anyway. Why is this function checking for a configuration file in ~/.printrun? AFAIK pronterface/pronsole look for a config file in either ~/.config/Printrun/pronsolerc, ~/.pronsolerc or ~/printrunconf.ini.

-rockstorm101

I suggest making an issue for that. The usage of the term configfile is ambiguously used as a global function and local variable (the term is only ever used in pronterface.py), it is only called as a function once, and used in a way that it doesn't make clear what is the expected result (It executes arbitrary code) and doesn't use any consistent location(s) you mentioned: exec(compile_file(configfile("custombtn.txt")), customdict) -Poikilos

As soon as someone figures out what custombtn.txt is and whether arbitrary code execution is even safe or expected here, we can figure out what to do:

• [ ] If someone can point out what the expected code may look like, or if the feature should be deprecated, that would be helpful. If kept, a comment seems like a good idea here, otherwise it could be doing anything from replacing the entire GUI to launching a rocket into space and no one would know whether to be surprised.
  • The only instance of custombtn.txt (via grep "custombtn\.txt" -r in the main branch) is not even very helpful:

printrun/pronterface.py:                    logging.warning(_("Note!!! You have specified custom buttons in both custombtn.txt and .pronsolerc"))
printrun/pronterface.py:                    logging.warning(_("Ignoring custombtn.txt. Remove all current buttons to revert to custombtn.txt"))

(Those two messages are also translated into 5 languages in pot and po files, but of course that doesn't say anything new...)

[volconst]

• otherwise it could be doing anything from replacing the entire GUI to launching a rocket into space and no one would know whether to be surprised.

If there is a rocket launch script on a computer ... https://quotefancy.com/quote/985524/Anton-Chekhov-If-there-is-a-gun-hanging-on-the-wall-in-the-first-act-it-must-fire-in-the

https://github.com/kliment/Printrun/blob/65ec26827191f2e08b932614ecaeee27ab497dbb/printrun/pronterface.py#L204-L206

[Poikilos]

If you do it that way, then custombnt.txt will just get moved again and again (the backup will be lost). I suggest:

                        if not os.path.exists("custombtn.old"):
                            os.rename("custombtn.txt", "custombtn.old")
                        # else ignore it. Overwriting below is OK since a backup exists.
                        rco = open("custombtn.txt", "w")
                        rco.write(_("# I moved all your custom buttons into .pronsolerc.\n# Please don't add them here any more.\n# Backup of your old buttons is in custombtn.old\n"))

• Also, remove the bare except statements--They are not PEP8 for a reason: Code (in any language) should handle specific exceptions specifically (for example, don't give up on the entire function without showing a warning message when rename or remove doesn't work). I can help with that code, but for other code necessary for the issue itself, I need more info:

Regarding rockstorm101's comment that I pasted into the original post:

• He was concerned it was the old location, but the old location is exactly what we want here since you've pointed out code that moves from the old location to the new one (via cbutton_save (save new) then os.rename (backup old)).
• The code you pointed out uses configfile to find it. Is "~/.printrun/" really where custombtn.txt used to be?

If yes, then maybe rename configfile to old_configpath for clarity. There could be a new configfile function, sure, but it is never used anywhere else (the appdirs module's user_config_dir function appears to take the place of it), and the code you pointed out only needs the old one.

IMO all of these functions ending in file should be renamed to end in path to make the function names self-explanatory: When I was first reading the code they appeared to be IO functions from the context, but they return a path not a file handle.

[volconst]

If you do it that way, then custombnt.txt will just get moved again and again (the backup will be lost).

Probably not, i think this will guard against it https://github.com/kliment/Printrun/blob/65ec26827191f2e08b932614ecaeee27ab497dbb/printrun/pronterface.py#L197-L198 See https://github.com/kliment/Printrun/issues/1236 for a probably broken custombtns.

IMO all of these functions ending in file should be renamed to end in path to make the function names self-explanatory

I sometimes want to rename functions, but refrain because I am afraid that some references can be missed. Have you used a tool that you are confident it won't break code.

[Poikilos]

Yeah, if people may use those functions for custombtns or other extensibility, I agree renaming them isn't good. So is it clear whether the "~/.printrun/" directory used in configfile is the real old directory? Or is the real old directory something like

. . . AFAIK pronterface/pronsole look for a config file in either ~/.config/Printrun/pronsolerc, ~/.pronsolerc or ~/printrunconf.ini"

?

Does anyone know?

[kliment]

cutombtn.txt has been deprecated for years. I think it's okay to break it now - it was there to migrate people's configurations so they wouldn't lose their macros when we deprecated it. All of those should now be migrated, so let's drop it.