-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Remove usage of project.documentation_type in tasks #4896
Remove usage of project.documentation_type in tasks #4896
Conversation
readthedocs/projects/tasks.py
Outdated
@@ -828,7 +831,7 @@ def is_type_sphinx(self): | |||
|
|||
# Web tasks | |||
@app.task(queue='web') | |||
def sync_files(project_pk, version_pk, hostname=None, html=False, | |||
def sync_files(project_pk, version_pk, doctype, hostname=None, html=False, |
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'm passing only the doctype
here rather than the whole config
object, we only use the doctype, but if someone thinks it's useful passing the config
I can change it.
Codecov Report
@@ Coverage Diff @@
## master #4896 +/- ##
=========================================
Coverage ? 76.86%
=========================================
Files ? 158
Lines ? 10042
Branches ? 1267
=========================================
Hits ? 7719
Misses ? 1983
Partials ? 340
|
Looks like I touched an untested code, need to figure out where to add the tests |
Need to add more tests for the test god |
The last commits added the missing tests here |
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 passing a dict of the validated config would be cleaner, but I don't have any strong opinions about it. If there are potentially any other cases where we'll need more access to the config dict in these tasks, lets just pass a dict object.
update_search( | ||
version.pk, | ||
commit, | ||
doctype=version.project.documentation_type, |
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.
@agjohnson this is the only place that I'm worried about passing a config object, we don't have one here.
We need to decide what to do with these projects: shouldn't we update this to say
We can also remove this chunk of code, https://github.com/rtfd/readthedocs.org/blob/4cf6a2ee8a86d6366e726ac53d696f3bae7dbdba/readthedocs/projects/models.py#L273-L277 Sentry issue: https://sentry.io/read-the-docs/readthedocs-org/issues/785965991/ |
@humitos I can do another PR to solve that. |
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.
Seems reasonable. Feels like a lot of work to be sending the doctype around, where we don't use it much. I wonder if we can make the update_search
function smarter so it can just figure out which type of docs they are, instead of passing it around a bunch before getting there.
Yeah, I'm getting more familiar with those bits of code here, but I guess we could kind of drop the usage of doctype to make some decisions here. |
This should be updated once #5121 passes and it should then be much simpler I think 👍 |
include_file=False, | ||
) | ||
Syncer.copy(from_path, to_path, host=hostname) | ||
# Always move PDF's because the return code lies. |
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 comment looks outdated
This depends on #4895, because we check if search/epub/pdf is supported by sphinx there
Ref #4638