Skip to content
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

Moved project tasks #4888

Closed
wants to merge 2 commits into from
Closed

Conversation

Alig1493
Copy link
Contributor

@Alig1493 Alig1493 commented Nov 11, 2018

Moved project tasks to places where they might be more appropriate.

Close #3775

@Alig1493
Copy link
Contributor Author

@stsewd I've made an initial pull request for issue #3775. Please have a look.

@codecov
Copy link

codecov bot commented Nov 11, 2018

Codecov Report

Merging #4888 into master will increase coverage by 0.04%.
The diff coverage is 83.03%.

@@            Coverage Diff             @@
##           master    #4888      +/-   ##
==========================================
+ Coverage   76.75%   76.79%   +0.04%     
==========================================
  Files         158      160       +2     
  Lines       10048    10073      +25     
  Branches     1265     1265              
==========================================
+ Hits         7712     7736      +24     
- Misses       1995     1996       +1     
  Partials      341      341
Impacted Files Coverage Δ
readthedocs/projects/admin.py 91.5% <100%> (ø) ⬆️
readthedocs/projects/tasks.py 69.92% <100%> (-1.28%) ⬇️
readthedocs/builds/tasks.py 79.45% <79.45%> (ø)
readthedocs/notifications/tasks.py 87.87% <87.87%> (ø)
tasks.py 79.16% <0%> (ø) ⬆️

msg='Creating ImportedFiles',
)
)
from readthedocs.projects.tasks import _manage_imported_files
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this isn't used on the projects.tasks module, so it can be moved too

)


# Random Tasks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can remove this comment now I guess

try:
requests.post(hook_url, data=data)
except Exception:
log.exception('Failed to POST on webhook url: url=%s', hook_url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the naming, none of these are part of the readthedocs.notification subsystem. These are still probably better off in a project.tasks.notifications or something unrelated to our notification system.

Eventually a refactor might combine these two concepts, but they are separate until then.

@agjohnson agjohnson added the PR: work in progress Pull request is not ready for full review label Nov 13, 2018
@agjohnson agjohnson added this to the Cleanup milestone Nov 13, 2018
@stale
Copy link

stale bot commented Jan 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Jan 10, 2019
@stale stale bot closed this Jan 17, 2019
@shivanshu1234 shivanshu1234 mentioned this pull request Feb 16, 2019
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress Pull request is not ready for full review Status: stale Issue will be considered inactive soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants