Closed stschiff closed 4 months ago
Attention: Patch coverage is 76.92308%
with 6 lines
in your changes are missing coverage. Please review.
Project coverage is 68.20%. Comparing base (
466870a
) to head (fc9cbdc
).
Files | Patch % | Lines |
---|---|---|
src/Poseidon/CLI/List.hs | 66.66% | 3 Missing and 3 partials :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for the review @nevrome, I have addressed the first comment by having introduced a new type. It's in ServerClient.hs
, see my response above.
Regarding the Web-API, I don't quite understand why we would like to cater for the case of using a new client with an old server? That should never happen. Even in the unlikely case that users actually use trident serve
locally, they would use a consistent server-client pair. And our server will always be up to date. So I don't see how the behaviour you discovered above would have any ramifications in production?
With respect to the additionalJannoColumns=ALL
-> I get your point, but I think introducing an additional option is also not great, because it would lead to illegal semantics, like: &additionalJannoColumns=Country,Publication&fullJanno
. Also, I am not even sure whether URLs even support boolean-like switch options like the one here. The usual way options are used is as key-value pair.
I think the rule that ALL
retains a special meaning as a column name is a good option considering all alternatives. It is simple, easy to remember and avoids illegal option-combinations. What do you think?
That the CLI introduces a special option `--fullJanno´ is not inconsistent in my view. A CLI is structured very differently than a URL, as it allows more expressiveness and hence can make things more explicit.
As stated in the meeting: You have convinced me :+1:
This PR introduces an API-flag
--fullJanno
for retrieving extended individual info with all standard janno columns. A similar option exists for the Web API, see Changelog.