Return 'other' if no standard account type is found in API #3165
Conversation
|
I like this better than mine 😄 . We should fix the test though but you can see what I did in my PR. |
| } | ||
|
|
||
| return null; | ||
| return 'other'; |
There was a problem hiding this comment.
| return 'other'; | |
| return 'domjudge_internal'; |
So other systems reading it know who to contact if they want to do anything with the account.
There was a problem hiding this comment.
I'm not strongly against this, but why is this better than just other? If you're querying the DOMjudge API, you know the source already. Do we want to distinguish our nonstandard accounts from possibly another system's nonstandard accounts?x
There was a problem hiding this comment.
The CDS might know the source, but things behind the CDS don't, if you make contest exports it's more convenient etc.
There was a problem hiding this comment.
I have a slight preference for other but am also fine with domjudge_internal.
There was a problem hiding this comment.
I also prefer other. I prefer a generic term that can potentially be added to the spec than something DOMjudge specific, as I don't see a case where you'd not know or easily figure out this came from DOMjudge.
eldering
force-pushed
the
fix-api-account-type
branch
from
5434f9e to
a89f0e3
Compare
Done |
| } | ||
|
|
||
| return null; | ||
| return 'other'; |
There was a problem hiding this comment.
I have a slight preference for other but am also fine with domjudge_internal.
Also rename variables and comment to clarify that the listed types are not the only ones allowed, just the ones suggested/ mentioned in the CLICS specification (as examples). Closes: #3163
eldering
force-pushed
the
fix-api-account-type
branch
from
a89f0e3 to
04078ab
Compare
Also rename variables and comment to clarify that the listed types are not the only ones allowed, just the ones suggested/ mentioned in the CLICS specification (as examples).
Closes: #3163