Page MenuHome

Allow using an external editor to write commit messages
AcceptedPublic

Authored by Isaac Weaver (wisaac) on May 22 2017, 11:27 PM.

Details

Summary

Instead of always passing the commit message from the command line with the -m flag, you can edit the commit message with an external editor (like svn or git). The editor can be configured with a global config file (~/.bamrc)

To configure an editor make the file look like this:

{
  "editor": "<editor command>",
  "editor_args": ["--optional", "--list", "--of", "--args"]
}

For example, to configure atom as the editor use:

{
  "editor": "atom",
  "editor_args": ["--wait"]
}

To configure vim use:

{
  "editor": "vim"
}

Diff Detail

Event Timeline

Isaac Weaver (wisaac) edited the summary of this revision. (Show Details)

Thanks for your patch, it seems like interesting functionality. There are a few places where the code can be improved, though.

bam/cli.py
130

The "global" in the name is misleading. Usually global configuration would be stored in /etc/bam_config. This is more "user config" or "home config".

The name .bam_config is rather unusual. I would suggest either using ~/.config/blender_asset_manager.json or ~/.bamrc or something else more in line with regular naming conventions. I especially like storing configuration in ~/.config, but that would have to be translated to something less OS-specific for Mac/Windows.

Also, the function needs a docstring to explain what it does.

135

The code will be less indented and easier to read when you invert the condition from "check it's okay" to "check there is an error":

if not os.path.exists(filepath):
    return {}

with open(filepath, 'r') as f:
    return json.load(f)

This clearly separates precondition checking from the actual functionality of the function.

953

Please move this block into its own functions. BAM already has too long functions for my taste.

964

Use the tempfile module instead. This manual temp file handling is too fragile, and bound to leave stray temp files in case there is an exception somewhere.

964

commit_msg is named confusingly, because it does *not* contain the commit message.

968

Include an explicit encoding when opening a file in text mode, or add a comment that explains why the platform-dependent encoding is used.

Also, don't use single-letter variables

969

Not all platforms use \n as line separator. If there is a reason for always using \n regardless of this, it needs to be explained in a comment.

982

Why not use for line in f and have Python do the line splitting logic for you?

985

No need to do an else after a break

986

String concatenation like this is inefficient, use a list and join() it outside the loop.

993

Just use if not message.strip() instead.

995

Is this the same status code as would be returned by argparse, when -m is required but not supplied? This should also be documented in a comment, as it should always be the same.

Sybren A. Stüvel (sybren) requested changes to this revision.May 23 2017, 10:42 AM
This revision now requires changes to proceed.May 23 2017, 10:42 AM
Isaac Weaver (wisaac) edited edge metadata.
Isaac Weaver (wisaac) marked 2 inline comments as done.
Isaac Weaver (wisaac) edited the summary of this revision. (Show Details)

Update with requested changes

I updated it with the requested changes.

bam/cli.py
130

A lot of the terminology comes from git and svn conventions. User-specific config files for git are stored in ~/.gitconfig and for svn they are stored in ~/.subversion/config. The only VCS I know of that uses the "rc" naming convention is mercurial.

The "global" also comes from git as a way to distinguished between repo-specific configurations and user-wide configs (e.g git config --global core.editor vim vs git config core.editor vim). At one point I thought about implementing a similar functionality to allow setting the editor in either the user-wide config or the .bam/config file but decided it would be better as a separate diff.

953

Not entirely related, but would it make sense to move the three "fake modules" to separate files? It might help make the main file a little cleaner.

969

That's a good point. It's used elsewhere in the file so it should probably be changed to os.linesep in all of those locations too.

Accepting the revision, with the note that Blender Asset Manager has been superseded (at least here in the Blender studio) by Blender Asset Tracer. BAM hasn't seen any development in a while.

This revision is now accepted and ready to land.Sep 27 2019, 5:36 PM