ietf-ribose / bibxml-service

Django-based Web service implementing IETF BibXML APIs
BSD 3-Clause "New" or "Revised" License
6 stars 4 forks source link

fix: provide missing adapter argument to resolve_mapping #18

Closed stefanomunarini closed 2 years ago

stefanomunarini commented 2 years ago

17

strogonoff commented 2 years ago

Ah, yes this is a problem (albeit not super critical, as xml2rfc mapping management interface isn’t heavily used right now).

However, there’s a bigger issue here. resolve_mapping() currently (after a refactor some time ago) returns 2 items, but the code requires to unpack 3 items, and it’ll remain the problem after the fix, right?

resolve_mapping() is typed as follows (and its implementation matches this typing):

def resolve_mapping(
    subpath: str,
    adapter: Xml2rfcAdapter,
) -> Tuple[
    Optional[BibliographicItem],
    Optional[str],
]

mypy actually complains about it (this is how I spotted it while reviewing):

Screenshot 2022-11-09 at 16 19 55

(One error shown is the one you fixed, but the other error remains and would cause the code to fail still.)

Unfortunately, I haven’t yet set mypy up to show me all issues across all files in the project, even those not currently open, so I suppose I have not been notified about this regression during my former refactor of resolve_mapping(). Currently mypy in my setup only shows issues in the currently open file.

Speaking of, maybe you could check your IDE configuration? If it’s PyCharm I’d expect it to show this error when you are editing xml2rfc_compat/management_views.py, considering that even my homegrown contraption of Neovim + mypy under LSP does it;)

strogonoff commented 2 years ago

PS. I’m OK if we do not fix this issue here and instead leave it, it would give us an example to verify how mypy linting on CI you implemented here reports typing errors.

stefanomunarini commented 2 years ago

However, there’s a bigger issue here. resolve_mapping() currently (after a refactor some time ago) returns 2 items, but the code requires to unpack 3 items, and it’ll remain the problem after the fix, right?

@strogonoff I've already fixed all the Mypy issues in the other PR (#13). This particular error is fixed here