-
Notifications
You must be signed in to change notification settings - Fork 355
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Added support for storage retrieval requests in VuFind core code
- added support in Voyager/VoyagerRestful drivers (call slips in Voyager land). - Extended Demo ILS driver to handle holds and storage retrieval requests properly. - Minor fixes to comments, etc.
- Loading branch information
1 parent
a17fbe3
commit 3bec0eb
Showing
53 changed files
with
2,741 additions
and
126 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this has already been committed but in general and moving forward I think we should avoid changing the names of public functions! When there's a strong argument to do so, maybe we could discuss / notify via the mailing list.
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do make note of things like this in the changelog of the wiki -- see https://vufind.org/wiki/changelog. In this particular case, I made the judgment call that the new name made more sense and the use of the function was fairly isolated... but perhaps it would have been better to be more cautious. There's also the possibility of adding a method with the old name that throws a deprecation warning as a wrapper. That just felt like overkill to me in this instance.
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there it is in the changelog! It's hard because different people are looking in different places. I don't have a habit of checking that page regularly, so for me an announcement on the mailing list would have saved a bit of oh-no-i-have-lost-code-somehow panic.
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, I agree the function is not widely used but I wouldn't be surprised if that is one of the more frequently edited templates.
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is true. I guess the way I've been looking at this is this:
1.) If you're working from master and have custom templates, you should have a procedure where whenever you do a merge, you look at all the templates that have changed and update your local custom versions to match. Otherwise, you'll run into problems down the road (breaks like this one, or at least missing functionality).
2.) If you're working from official releases, you should look at the change log whenever you upgrade and it will warn you of major issues to worry about. A large-scale diff a la #1 is still probably a good idea.
Not saying that entirely justifies making major changes without guilt -- but that's how I rationalized this one. And point #1 is really important. This is the one thing that troubles me about VuFind's convenient separation of local changes -- you really have to be vigilant about certain types of core changes or else you can run into problems. It would be really nice if there were some sort of magic version control mechanism that could say "file y is derived from file x; file x just changed -- you'd better do a merge" but as far as I know, that's a little too advanced for existing systems.
Anyway, I'm always open to suggestions for improvement!
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is perfectly fair for me to bring up what is practically a law of software development (don't change the signature of a public method).
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion for improvement is as follows: try to post backwards-incompatible changes more widely. Maybe mention them on dev call minutes and post them to the dev list. Thanks for your consideration.
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you're absolutely right -- sorry if I seemed to be dismissing that point. You just got me off on a tangent thinking about the general challenges of maintaining local themes (even in the absence of gotchas like this particular one). I've been feeling for a while like we need a tool to help with this, but I'm not sure that one exists.
In any case, you've persuaded me that it's worth the very low overhead of adding a getHoldUrl() wrapper method around getRequestUrl() to maintain backward compatibility and prevent confusion. Just pushed that up.
I also appreciate the feedback -- I'm finding it increasingly difficult to review new code as thoughtfully and thoroughly as I used to (and would like to) because the volume keeps increasing, but I also feel pressure to keep things flowing since there's nothing worse than a stale pull request! Any time others have to put eyes on things is extremely valuable.
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, in addition to that, I'll make a greater effort to be more consistent about reporting future potential-backward-breakers in more venues. I'll try to remember to make a policy that, if it's important enough to add to the changelog, it's important enough to announce elsewhere.
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this. Your maintenance of this project never fails to impress. I actually would like to try to keep a closer eye on PRs and code changes. I will mess with my notification settings.
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take part of the blame here. I actually had adding a compatibility wrapper in my mind at some point, but somehow forgot to bring that up. I apologize for the the trouble.
3bec0eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a problem -- it's good to see that things like this get caught before we go to release! (Thanks again, Anna!).