-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extracting Qa.authority_for
#379
Open
jeremyf
wants to merge
5
commits into
main
Choose a base branch
from
jeremyf---extracting-logic-for-determining-qa-authority
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Extracting Qa.authority_for
#379
jeremyf
wants to merge
5
commits into
main
from
jeremyf---extracting-logic-for-determining-qa-authority
Conversation
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
jeremyf
force-pushed
the
jeremyf---extracting-logic-for-determining-qa-authority
branch
from
December 5, 2022 22:06
259042f
to
300fb6e
Compare
Prior to this commit, we used different initialization logic for linked data authorities and non-linked data. Further, the initialization logic was locked away inside of a controller. With this commit, we expose a method that provides consistent initialization logic; albeit with different returned classes that have slightly different implementation nuances. The application can then rely on that and we can share initialization logic across controllers and also expose a method that allows non-controller initialization to begin to follow the same logic pathway. Where are the tests? The initializations are covered by controller tests, so I have chosen not to write new ones. There is further work to do because LinkedData and non-LinkedData there are different signatures for `find` and `search`. Ideally, we would normalize the public facing implementation logic. However, this commit preserves backwards compatibility, simply introducing a new feature.
jeremyf
force-pushed
the
jeremyf---extracting-logic-for-determining-qa-authority
branch
from
December 7, 2022 18:14
f359447
to
24776bc
Compare
Prior to this commit, the returned value from `Qa.authority_for` was something that was inconsistent. Depending on the controller (or authority), the `find` and `search` would require different parameters. With this commit, those nuances are encapsulated in their own methods. This begins to work towards a more foundational promise of questioning authority; namely that the end point that asked questions of QA didn't need to know which type of authority it was. That promise was partially delivered at the routes level but this commit pushes that down to the internal API level. As a bonus, and perhaps confusion, I've included the `Qa::AuthorityRequestContext` as an interface to help convey how one might provide a different (non-controller) context to instantiating and authority.
jeremyf
force-pushed
the
jeremyf---extracting-logic-for-determining-qa-authority
branch
from
December 9, 2022 20:06
e962cf8
to
1ff3d8a
Compare
Prior to this commit, we always assumed that each "find" would request from `https://id.loc.gov/authorities`. With this commit, we use the "subauthority" to inform what the base URL is for the `https://id.loc.gov/` host. In some cases this is `authorities` and in others it's as the code indicates. Hopefully works to close the following: - scientist-softserv/utk-hyku#267
This was referenced Dec 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracting
Qa.authority_for
24776bc
Prior to this commit, we used different initialization logic for linked
data authorities and non-linked data. Further, the initialization logic
was locked away inside of a controller.
With this commit, we expose a method that provides consistent
initialization logic; albeit with different returned classes that have
slightly different implementation nuances. The application can then
rely on that and we can share initialization logic across controllers
and also expose a method that allows non-controller initialization to
begin to follow the same logic pathway.
Where are the tests? The initializations are covered by controller
tests, so I have chosen not to write new ones.
There is further work to do because LinkedData and non-LinkedData there
are different signatures for
find
andsearch
. Ideally, we wouldnormalize the public facing implementation logic. However, this commit
preserves backwards compatibility, simply introducing a new feature.
Introducing
Qa::AuthorityWrapper
baf399f
Prior to this commit, the returned value from
Qa.authority_for
wassomething that was inconsistent. Depending on the controller (or authority), the
find
and
search
would require different parameters.With this commit, those nuances are encapsulated in their own methods.
This begins to work towards a more foundational promise of questioning
authority; namely that the end point that asked questions of QA didn't
need to know which type of authority it was.
That promise was partially delivered at the routes level but this commit
pushes that down to the internal API level.
As a bonus, and perhaps confusion, I've included the
Qa::AuthorityRequestContext
as an interface to help convey how onemight provide a different (non-controller) context to instantiating and
authority.
WIP Reworking authority request context
4fd4907
require context and update logic of fetch/search header methods
d3c51a5
Adding a
Qa::Authorities::Loc.type_for
79a13f9
Prior to this commit, we always assumed that each "find" would request
from
https://id.loc.gov/authorities
.With this commit, we use the "subauthority" to inform what the base URL
is for the
https://id.loc.gov/
host. In some cases this isauthorities
and in others it's as the code indicates.Hopefully works to close the following: