Implement post_prefix and post_suffix consistently for all clients

[wilddeej]

Includes tests to verify new/updated functionality.

[wilddeej]

In fact, my first attempt implemented it exactly as you describe, but that didn't work; putting it on a separate line also put it in a separate shell that didn't carry the definition forward into the tests, and it failed. Note that this was in the context of executing the tests within a Docker container, where the MEDIA_DIR env var had been set to a different value. It might work just fine in other contexts, but I don't think the env var value will actually be used within the test invocation.

[aurelg]

Thanks for this new PR!

I notice you often use self._post_suffix if self._post_suffix else ''. It makes sense, since it is initialized with None (GenericClient.py:26). An alternative could be to initialize it with '', so that its value does not have to be tested against None in each client. The result would be a simpler / more readable code. This '' would be instantiated when the GenericClient class is parsed, and would be shared by all its instances as a class attribute, unless overridden by line 238.

(This applies to _post_prefix as well).

BTW, I noticed a possible issue with the declaration of _tags, _tag_filter_opts and _url_shortener_opts at lines 18, 19 and 22 in GenericClient.py. This list and these two dicts are instantiated when parsing the file. It has no side effect, since they are assigned later a new value, if needed (lines 203, 209 and 244, respectively). However, this opens the door to possible mistakes later, just like this one. IMHO, just in case, they should be set to None instead.

[wilddeej]

Excellent points. I've implemented all as suggested.

[wilddeej]

The change to the class declaration of _tags, _tag_filter_opts, and _url_shortener_opts have broken all of the tests. I suspect this is because the set_common_opts function is not called by any of the tests, and so these variables can never get properly initialized. I see four possibilities:

1. Change a lot of code to correctly respond to values of None for these variables,
2. Change all tests to call set_common_opts,
3. Introduce an __init__ function into the GenericClient where these values will be initialized,
4. Put the potentially dangerous class initialization back in place

Which do you think is best? I lean towards the last option, with an Issue opened up against the underlying problem(s).

[aurelg]

1. Change a lot of code to correctly respond to values of `None` for these variables,

This would certainly be the cleaner option.

2. Change all tests to call `set_common_opts`,

If a call to set_common_opts is mandatory, then this call should be enforced at some point. I don't think calling it "manually" really makes sense.

3. Introduce an `__init__` function into the `GenericClient` where these values will be initialized,

This would be my second choice, and I can live with it if you don't want to update the code to deal with None.

4. Put the potentially dangerous class initialization back in place

I'd say that counting on something dangerous to solve a temporary problem is a dangerous bet. :-)

Edit: An __init__ constructor in the GenericClient would need to be explicitly called from its subclasses.

[wilddeej]

All of the clients already call set_common_opts in their __init__ functions, but that doesn't happen within tests, which have a test-specific initialization function injected. Based on that, it doesn't sound like option 3 is viable, and wouldn't solve the problem in the tests anyway. Both options 1 and 2 are really beyond the scope of this work, even though it might be the "right" thing to do. I think it would be better to solve that in a more targeted effort, rather than trying to fit it in here.

[wilddeej]

I created Issue #49 for the client var cleanup issue, and this is fixed in PR #50 (including the "else" implementation of these variables as mentioned above). As it turns out, this really did not require changes to "a lot of code" as originally anticipated.