Second attempt at fixing #13722 and refactoring alphanumeric range code

[pv2b]

Fixes: #13722

This pull request replaces my first train-wreck of an attempt in #13728.

The first two commits are preparatory refactoring work to improve the testability of current code.

First, I identified an issue where the tested behavious wasn't the actual behaviour. That's because expand_alphanumeric_pattern was only called from ExpandableNameField.to_python, which did its own regex matching on the input string before patting it over to expand_alphanumeric_pattern. It makes a lot more sense for that regex matching to only happen in expand_alphanumeric_pattern, so that was moved to that function, which, it turned out, was already doing it implicity by string splitting on the same regex.

Doing that change in turn made it possible to further simplify expand_alphanumeric_pattern by removing some unneccessary extra checks.

However, this caused some test failures. This is due to the fact that expand_alphanumeric_pattern's behaviour was changed. Before, it would throw an exception if there are no patterns in it, now it instead returns the string unmodified, which is exactly what the appication as a whole was doing, but now it's all contained in that function. That made it neccessary to adjust the unit tests to reflect this change of responsibility.

Once the prep-work was done, I replaced parse_alphanumeric_range with an almost entirely new implementation which I believe is cleaner and easier to read. This is what actually fixes the bug. It also adds some better error messages for the user when something goes wrong.

During the work in refactoring, I discovered another unrelated bug that also somehow made it into the test suite as a feature. For some reason, entering a string like r[a-9]a would make expand_alphanumeric_pattern return an empty list. This behaviour made no sense to me, so again, I changed the tests. This shouldn't have changed the external behaviour of the application, since when an empty list was received by to_python, the "name" field just ended up be empty, which in turn caused the form validation to fail with an incorrect message that the field was required. This way, there's a clearer error message as to what's going on.

There's still some improvements that could be done here, for example, I don't like how a pure utility function throws forms.ValidationError, but I'm not going to change it because it's not really causing any issues at the moment, and it's easy enough to change in the future if it ever becomes a concern.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

[github-actions[bot]]

This PR has been automatically closed due to lack of activity.

[DanSheps]

Re-opening at the request of Peter.

[pv2b]

Honestly, I think you are trying to fix too much at once.

On the contrary, the changes to the unit tests are in an effort to not fix current bugs, but instead preserve current behaviour.

Fixing all the "bugs" would make the patch a lot cleaner, but it would also mean changing a lot of behaviour in one patch, which, ironically, is what you're trying to avoid :-)

[DanSheps]

Honestly, I think you are trying to fix too much at once.

On the contrary, the changes to the unit tests are in an effort to not fix current bugs, but instead preserve current behaviour.

Fixing all the "bugs" would make the patch a lot cleaner, but it would also mean changing a lot of behaviour in one patch, which, ironically, is what you're trying to avoid :-)

As I mentioned, which you seemingly ignored, all you need to "fix" this specific bug is:

if begin == end:
    values.append(begin)

after if begin.isdigit() and end.isdigit()

We could also simplify this to:

if begin == end:
   ...
elif begin.isdigit() and end.isdigit():
   ...
else:
   ...

and remove the begin==end logic from the else statement.

[pv2b]

Honestly, I think you are trying to fix too much at once.

On the contrary, the changes to the unit tests are in an effort to not fix current bugs, but instead preserve current behaviour. Fixing all the "bugs" would make the patch a lot cleaner, but it would also mean changing a lot of behaviour in one patch, which, ironically, is what you're trying to avoid :-)

As I mentioned, which you seemingly ignored, all you need to "fix" this specific bug is:

if begin == end:
    values.append(begin)

after if begin.isdigit() and end.isdigit()

We could also simplify this to:

if begin == end:
   ...
elif begin.isdigit() and end.isdigit():
   ...
else:
   ...

and remove the begin==end logic from the else statement.

Okay, let's back up a bit here. There's no reason for us to get confrontational over this. I didn't intend to ignore anything. I probably just forgot to actually write that I agreed with it in my effort to explain what I believed was a misunderstanding in your review.

Anyway, a simpler fix probably would have sufficed, but at the time I wrote this code a few months ago, I took the opportunity to clean things up, and in doing so, I uncovered a few more bugs, and now here we are. I think my patch as written makes Netbox better, but if you want to reject it as too big of a change, I understand.

So, if you'd prefer to abandon this refactor and just introduce a short fix along the lines you outlined, that's fine. Let me know and I'll make a new branch like that, or if you'd rather do it yourself, that's fine with me too.

But if you'd for me to finish the PR in the state it is now, I'm happy to make sure it merges. If you want me to revert the unit tests to how they used to be and instead change the code's behaviour, so change how things like r[9-]a are handled to make them throw errors, to match the "intent" of the unit tests, I'll be happy to do that too. Ultimately there's no right and wrong here, just a decision to be made.

So just let me know how you want me to proceed (or not) and we'll take it from there.

[jeremystretch]

Thank you for your effort, however the root bug is quite simple and has been resolved in PR #15301.