Changeset View

Standalone View

bam/cli.py

Context not available.
	return None	return None

	@staticmethod	@staticmethod
		def load_global_config():
		filepath = os.path.expanduser('~/.bam_config')
		sybrenUnsubmitted Not Done Inline Actions 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. sybren: The "global" in the name is misleading. Usually global configuration would be stored in…
		wisaacAuthorUnsubmitted Not Done Inline Actions 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. wisaac: A lot of the terminology comes from git and svn conventions. User-specific config files for git…
		if os.path.exists(filepath):
		with open(filepath, 'r') as f:
		return json.load(f)
		else:
		return {}
		sybrenUnsubmitted Done Inline Actions 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. sybren: The code will be less indented and easier to read when you invert the condition from "check…

		@staticmethod
	def load(id_="config", cwd=None, abort=False):	def load(id_="config", cwd=None, abort=False):
	filepath = bam_config.find_basedir(	filepath = bam_config.find_basedir(
	cwd=cwd,	cwd=cwd,
Context not available.
	fatal("Path not a project session, (%r)" %	fatal("Path not a project session, (%r)" %
	session_rootdir)	session_rootdir)

		if message is None:
		sybrenUnsubmitted Done Inline Actions Please move this block into its own functions. BAM already has too long functions for my taste. sybren: Please move this block into its own functions. BAM already has too long functions for my taste.
		wisaacAuthorUnsubmitted Not Done Inline Actions 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. wisaac: Not entirely related, but would it make sense to move the three "fake modules" to separate…
		import subprocess
		from contextlib import redirect_stdout

		global_config = bam_config.load_global_config()
		editor = global_config.get('editor')
		editor_args = global_config.get('editor_args')

		if editor is None:
		fatal('Either add a --message flag or specify an editor in the global config')

		commit_msg = os.path.join(basedir, 'bam_commit.tmp')
		sybrenUnsubmitted Not Done Inline Actions 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. sybren: Use the tempfile module instead. This manual temp file handling is too fragile, and bound to…
		sybrenUnsubmitted Not Done Inline Actions `commit_msg` is named confusingly, because it does not contain the commit message. sybren: `commit_msg` is named confusingly, because it does not contain the commit message.

		ignore_line = "--This line, and those below, will be ignored--"

		with open(commit_msg, 'w') as f:
		sybrenUnsubmitted Not Done Inline Actions 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 sybren: Include an explicit encoding when opening a file in text mode, or add a comment that explains…
		f.write("\n" + ignore_line + "\n\n")
		sybrenUnsubmitted Not Done Inline Actions 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. sybren: Not all platforms use `\n` as line separator. If there is a reason for always using `\n`…
		wisaacAuthorUnsubmitted Not Done Inline Actions 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. wisaac: That's a good point. It's used elsewhere in the file so it should probably be changed to `os.

		with redirect_stdout(f):
		bam_commands.status(paths)

		cmd = [editor]
		if editor_args:
		cmd.extend(editor_args)
		cmd.append(commit_msg)
		subprocess.run(cmd)

		with open(commit_msg, 'r') as f:
		message = ''
		for line in f.read().split('\n'):
		sybrenUnsubmitted Not Done Inline Actions Why not use `for line in f` and have Python do the line splitting logic for you? sybren: Why not use `for line in f` and have Python do the line splitting logic for you?
		if line.strip() == ignore_line:
		break
		else:
		sybrenUnsubmitted Not Done Inline Actions No need to do an `else` after a `break` sybren: No need to do an `else` after a `break`
		message += line + '\n'
		sybrenUnsubmitted Not Done Inline Actions String concatenation like this is inefficient, use a list and join() it outside the loop. sybren: String concatenation like this is inefficient, use a list and join() it outside the loop.
		# Remove trailing newline
		message = message[:-1]

		# Remove the commit message file
		os.remove(commit_msg)

		if message.strip() == '':
		sybrenUnsubmitted Not Done Inline Actions Just use `if not message.strip()` instead. sybren: Just use `if not message.strip()` instead.
		print("Empty commit message, aborting")
		sys.exit(1)
		sybrenUnsubmitted Not Done Inline Actions 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: Is this the same status code as would be returned by `argparse`, when `-m` is required but not…

	# make a zipfile from session	# make a zipfile from session
	paths_uuid = bam_session.load_paths_uuid(session_rootdir)	paths_uuid = bam_session.load_paths_uuid(session_rootdir)
Context not available.
	)	)
	subparse.add_argument(	subparse.add_argument(
	"-m", "--message", dest="message", metavar='MESSAGE',	"-m", "--message", dest="message", metavar='MESSAGE',
	required=True,
	help="Commit message",	help="Commit message",
	)	)
	subparse.add_argument(	subparse.add_argument(
Context not available.