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.
Add modeling for intersphinx data #5289
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
Add modeling for intersphinx data #5289
Changes from 1 commit
83c6f40
db5fbd8
c2cd164
41b2857
806d3c2
0f82c26
639b219
67d909a
537c66a
647ae3f
5b7d599
1dfabe8
08296ae
1e087fb
12bdf04
8e015c9
444e739
1672421
462545e
f53e589
98aae46
c934046
96fa45a
d8d75d5
09b3c80
0da9283
1bc31d0
97851c4
021e12d
ab7a30c
fa72754
4ea9121
ec71993
18877c1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a small comment with previous to this line, with the expected structure of this
einfo
object. Likeor... a link to the docs.
I found debugging this complicated later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use urlparse https://docs.python.org/3/library/urllib.parse.html, but the logic here is very simple, so not really sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, feels unnecessary. It's also not a full URL, so it might be confused by it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it doesn't matter too much, but to keep consistency here with the other endpoints, I'd say that it should be called
sphinxdomains-detail
(without the_
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is autogenerated by REST framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, yes... but it comes from the
register
: https://github.com/rtfd/readthedocs.org/pull/5289/files/021e12d9472f3e4cb1f7f69413e0af0e25808c86..ab7a30cf1cb40012ce71704824723aa5c1597b67#diff-48d1599aaf2a6be451a9f14bdf0cd81aR38I should have commented that other line instead :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. It also shouldn't be plural. Fixed.