@ChanceNCounter did I understand the testcase correctly that the check for attribute error was mainly meant to verify that the key didn't exist in the object?

@ChanceNCounter did I understand the testcase correctly that the check for attribute error was mainly meant to verify that the key didn't exist in the object?

So sorry! I didn't notice the mention.

Those checks, unless I've gotten completely lost in the weeks since, were to verify that parse_en._ReplaceableNumber actually throws the exception.

I made no attempt to check the sanity of existing code during that testing binge. I just got mad at Coveralls, and started writing direct tests for error-handling behavior that's hard to invoke "accidentally" on purpose.

Ok then I seem to have misunderstood the purpose of the line.

So expected behavior of the object is that it throws the AttributeError if a setattr call is made with a non-existing member? Which looks weird since setattr shouldn't do that according to the python docs and you internally catch the AttributeError that getattr could produce.

On the other hand how is the test I made even working? Since the setattr "key" is only performed once it shouldn't really raise the Immutable error...

I updated the PR to what makes sense to me...

Create a new entry, make sure that the exception is raised when a modification attempt is made. Is this good enough?

I agree with you that the test you wrote should no longer be covering line 132.

I don't fully understand the exception handling myself. I didn't write the code, I just wrote the tests to invoke it. It looks like @ChristopherRogers1991 wrote lines 132-135 containing the relevant tests.

I defer to him, and to Coveralls, and to your good sense, assuming nothing breaks in the wild.

The test added here (setting the value, then expecting the error if you try to change it) looks correct - the expectation is that once an attribute on the object has been set, it cannot be changed. I do have two very small notes, though:

1. It's a little odd that this uses setattr, rather than just setting the value - replaceable.key = None would be the same thing.

2. There would probably be more value in checking that value and tokens cannot be changed. Those are the two fields set in the constructor, so you could just construct the object and then try to set either those.

Do we need to test both exceptions?

Define "need." I wouldn't say that any of these tests are strictly necessary, but at this point it's pretty inexpensive to test, and mitigates some risk of breaking things. I'd say if we're going to test it at all, we might as well test both values.

Thanks @ChristopherRogers1991 and @ChanceNCounter for the guidance, I've updated the commit to check the pre-existing values along with that creating a new member works as expected.

Since there's been no more comments after my last changes I'm going to assume this test now looks ok. So I'll merge this in now.