general: upstream invenio-pidstore #1613
Conversation
|
|
||
| def get_pid_type_for(endpoint): | ||
| """Get the pid_type code corresponding to the recid of the given endpoint.""" | ||
| return RECORDS_REST_ENDPOINTS[endpoint]['pid_type'] |
There was a problem hiding this comment.
This is called with input coming from the users (e.g. the /ajax/references view function), therefore I'd recommend wrapping it in a try/except with a default.
There was a problem hiding this comment.
shouldn't the caller code be wrapped? (i.e. the one that is directly receiving the input from the user?)
There was a problem hiding this comment.
In particular there is no good default, and it would hide wrong usages. I'd prefer to bubble up the exception to the calling code which is really aware of the origin of the endpoint argument and should know how to act on a KeyError.
There was a problem hiding this comment.
In this case I'd say it's equivalent, because this is not a whitelist/sanitization/security issue, just a potential source of exceptions on Sentry. Your call!
Changed my mind. You're right!
|
This helper (used in the API) is using the opposite mapping from the one you are now introducing. I'd say we should also have in the config the "inverted" dictionary generated by a dictionary comprehension. EDIT: corrected the broken link |
jacquerie
changed the title
|
This is another place where you could use the inverse mapping. |
|
Put it in WIP because it's missing a few uses of the |
The link behind This helper is broken. What did you want to link? |
This one was really a blocker IMHO because our fork is changing the SQLAlchemy model and wouldn't therefore allow for an easy upgrade to the upstream one. |
Whoops, fixed. |
kaplun
force-pushed
the
upstream-pidstore
branch
from
3f74594 to
fec6e4a
Compare
|
I think I have implemented your requests. P.s. OT why haven't you used the Github review for this PR? Just by chance or you don't like it? |
I love it! But in this case my comments were about things that were missing from your PR, therefore I had no way of commenting on those... |
kaplun
force-pushed
the
upstream-pidstore
branch
2 times, most recently
from
89a4ce8 to
2d5874e
Compare
|
Gaah: it's failing only on acceptance tests. I wonder what I am missing at this point? |
kaplun
force-pushed
the
upstream-pidstore
branch
from
2d5874e to
0ff6af8
Compare
* No longer uses INSPIRE fork of invenio-pidstore. * Introduces new get_pid_type_for() and get_endpoint_from_pid_type() helpers to map between endpoints and the corresponding pid_types, now that pid_types are constrained in length. (closes inspirehep#1579) Signed-off-by: Samuele Kaplun <[email protected]>
kaplun
force-pushed
the
upstream-pidstore
branch
from
0ff6af8 to
02b4651
Compare
kaplun
changed the title
|
Sent a new version using upstream |
|
What's the status on this? This was a good thing. |
|
Ah nothing, I need focus on it to make it work. I might do it today. |
to corresponding pid_types, now that pid_types are constrained in
length.
(closes Upstream pidstore #1579)
Signed-off-by: Samuele Kaplun [email protected]