Add ?censor and ?safe params

[odinhb]

Closes #264

[ngerakines]

Really like the direction this is going on. I have some small changes and feedback that I'll get to next week.

[odinhb]

I'm still curious about the feedback on this one. I'm not a python developer by day so it would be appreciated.

[ferdnyc]

Some thoughts, all take-it-or-leave-it stuff of course. Some bikeshedding, but not too much (I hope).

[ferdnyc]

Suggested change

	grawlix_chars = censor if type(censor) == str else GRAWLIX
	grawlix_chars = censor if isinstance(censor, str) else GRAWLIX

[ferdnyc]

Just FYI, for the future, this can be done in one statement as:

with (
    open("commit_messages/safe.txt", "w") as safe_f,
    open("commit_messages/unsafe.txt", "w") as unsafe_f,
    open("./commit_messages.txt") as original_f,
):

...And then you don't need any explicit .close()es.

(Python 3.10+ is required to surround the open()s with ()s, otherwise they all need to be on one line.)

[ferdnyc]

Having all these links just... kind of "dumped" here might be confusing, especially since the permalink and the others are completely different animals. (The other three links are completely unconnected to the current commit reason being shown.) Having them all together feels sort of like it's implying that the link goes to a "safe only or "censored" version of the current message. At least, it could be misinterpreted that way.

I wonder if it would look cleaner (and be clearer to users) to separate the site-links out a bit from the current output, and maybe add some sort of heading? e.g:

<h3>Get another</h3>
<ul class="link-list">
  <li class="link-wrapper"><a href="/">unsafe</a></li>
  <li class="link-wrapper"><a href="/?safe">safe only</a></li>
  <li class="link-wrapper"><a href="/?censor">censored</a></li>
</ul>

(The UL can always be styled to remove the bullets:)

.link-list { list-style: none; }

[ferdnyc]

Suggested change

	Name: Odin Heggvold Bekkelund (github: odinhb)
	Name: Odin Heggvold Bekkelund (github:odinhb)

😉

[ferdnyc]

Suggested change

	with open(humans_file, 'r', encoding='utf-8') as f:
	for line in f:
	if "Name:" in line:
	data = line[6:].rstrip()
	if data.find("github:") == 0:
	names.append(data[7:])
	else:
	names.append(data.split(" ")[0])
	def _extract_name(name):
	if name.startswith("github:"):
	return name.removeprefix("github:")
	return name.split(" ")[0]
	with open(humans_file, 'r', encoding='utf-8') as f:
	names = [
	_extract_name(line.rstrip().removeprefix("Name: "))
	for line in f
	if line.startswith("Name: ")
	]

List comprehensions are much faster than looping .append()s, as they avoid repeated list reallocations. And then you don't need to create the empty list variable in advance.

(Though IMHO the code to extract the names should be in a function that returns the list, and names should be defined in one go with names = _read_name_list(humans_file).)

str.removeprefix is Python 3.9+, if that's too high a minimum version the string-index math is still an option.

[ferdnyc]

Ditto:

Suggested change

	with open(safe_templates_file, 'r', encoding='utf-8') as f:
	for line in f:
	_hash_and_store(line, safe_templates)
	with open(unsafe_templates_file, 'r', encoding='utf-8') as f:
	for line in f:
	_hash_and_store(line, unsafe_templates)
	with open(censorable_templates_file, 'r', encoding='utf-8') as f:
	for line in f:
	_hash_and_store(line, censorable_templates)
	with open(safe_templates_file, 'r', encoding='utf-8') as f:
	safe_templates = {
	_hash_template(line): line
	for line in f
	}
	with open(unsafe_templates_file, 'r', encoding='utf-8') as f:
	unsafe_templates = {
	_hash_template(line): line
	for line in f
	}
	with open(censorable_templates_file, 'r', encoding='utf-8') as f:
	censorable_templates = {
	_hash_template(line): line
	for line in f
	}

...And you can get rid of _hash_and_store() completely.

[ferdnyc]

Blank line before this call, please. (black and safepep8 will actually tell you they want two blank lines on either side of it. But at least one.)

[ferdnyc]

A documentation comment would be helpful here, because I have no idea what this function is doing.

[ferdnyc]

There's some argument to be made that all of the *TF messages also belong in the unsafe file. They're not really usefully censorable, but they're not really censored per se, either.