Subversion interactive support

[johnthagen]

Subtask of #6386

@cjerdonek could you tag this as Trivial?

[johnthagen]

@cjerdonek Could you look over the implementation before I start adding tests? Also, any ideas as to why a few tests are failing? I couldn't see how what I changed in the Subversion constructor would cause the regression.

[cjerdonek]

Also, any ideas as to why a few tests are failing?

It's probably because the url is being passed positionally as the first argument in other code, and so it's being used as the use_interactive argument rather than the first args argument: def __init__(self, use_interactive=None, *args, **kwargs):. You can see some of the test failures are showing that url is None, which means the url isn't getting passed to the VersionControl base class.

Could you look over the implementation before I start adding tests?

Will do later..

[johnthagen]

It's probably because the url is being passed positionally as the first argument in other code

You were right. I've fixed up the constructor signature to fix this.

[johnthagen]

@cjerdonek

I also think the class should grow a private _vcs_version instance attribute to cache the version value from the method you just wrote. It can initially be set to None (unset), with a None version value being stored as some other non-None value (perhaps False)

I used a separate _call_vcs_version_failed attribute because I think it composes better with the type system. That way _vcs_version can be Optional[Tuple[int, ...]] rather than Union[Optional[Tuple[int, ...]], bool].

[johnthagen]

@cjerdonek Presuming the CI passes, this PR should be ready for review. Tests have been added.

[cjerdonek]

Thanks! Some quick comments (not exhaustive).

[johnthagen]

@cjerdonek I believe all of your comments have now been addressed.

[johnthagen]

@cjerdonek Was wondering if you had any more comments for this PR. Thanks.

[cjerdonek]

I'll get to it. Just try to be patient..

[cjerdonek]

Thanks for your patience. Here are some additional review comments.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[johnthagen]

@cjerdonek Thanks for the review.

Presuming the CI passes, I believe that I have addressed all of your comments.

[cjerdonek]

Strange, this was already fixed in the latest PR before this review. Perhaps you are commenting on an older review?

The explanation on this: I had some "pending" comments from before that I didn't publish because I decided to wait instead for your next version. And then when I reviewed your newer version, not all of the pending comments were still applicable (the GitHub UI hid the ones that were marked "outdated," but they got published anyways when I clicked okay).

[johnthagen]

@cjerdonek Ah, that makes sense.

[cjerdonek]

Looking good! Just a few more (final?) comments..

[johnthagen]

@cjerdonek I believe all of your review comments have been addressed.

[cjerdonek]

@johnthagen I squashed / rebased your changes into two commits (one with the code changes, and one with the method moves) to make it easier for people to see, and also made a few tiny tweaks, if that's okay.

[johnthagen]

@cjerdonek Sounds great, thanks!

[cjerdonek]

Great, and thanks again for sticking with this, @johnthagen!

[johnthagen]

@cjerdonek Of course! Thanks for all of the detailed reviews.