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

Enforce pylint violation when using old event tracker. #6080

Merged
merged 1 commit into from
Dec 3, 2014

Conversation

benpatterson
Copy link
Contributor

This is a deprecated use and we need to be sure others don't adopt it.

@benpatterson
Copy link
Contributor Author

Idea is this will raise our pylint violations slightly today but if anyone decides to re-use the library, diff-quality will fail. it doesn't appear to catch all cases at this point, though, because of pylint failures to import. i'll see how the jenkins builds handle it.

CC @mulby

@benpatterson
Copy link
Contributor Author

@sarina in order to prevent folks from using a deprecated event-tracking module, I'm introducing this rule change. My end goal is to prevent additional drift or re-use of this module. Thoughts?

FYI this adds 8 pylint violations.

@sarina
Copy link
Contributor

sarina commented Dec 1, 2014

@benpatterson I think that definitely makes sense. Where are the 8 pylint violations coming from?

I think it's not going to be possible for us to ever have 0 pylint violations. I think a reasonable number is somewhere around 500. so on that front I'm ok. but if it's possible to explicitly ignore some of the introduced violations (because we know we're ok with them) maybe we could do that here.

@benpatterson
Copy link
Contributor Author

@sarina
common/djangoapps/student/views.py
common/djangoapps/util/views.py
lms/djangoapps/instructor_task/tasks_helper.py
lms/djangoapps/lms_migration/migrate.py
lms/djangoapps/dashboard/sysadmin.py
lms/djangoapps/courseware/module_render.py
and a couple test files under common

I think we should not comment these out though. The library is deprecated, so the violations are technically valid.

@sarina
Copy link
Contributor

sarina commented Dec 2, 2014

OK, makes sense.

Is there a backlog ticket to migrate these files away from using a deprecated library? Why are they using deprecated libraries?

@benpatterson
Copy link
Contributor Author

@sarina I did a quick scan and don't see a ticket, but let's doublecheck... @mulby do we have a backlog ticket for upgrading these modules to use the latest event tracker? (Or the shim?)

Sarina, they're using deprecated libraries because of old-fashioned tech debt, I believe.

@sarina
Copy link
Contributor

sarina commented Dec 3, 2014

OK, makes sense. Would be good, but not strictly necessary, to capture that somewhere - but I agree, best to have the pylint warnings so if someone goes in to those files to clean up perhaps they'll fix. 👍

@mulby
Copy link
Contributor

mulby commented Dec 3, 2014

Found the ticket

https://openedx.atlassian.net/browse/AN-1516

@benpatterson
Copy link
Contributor Author

Solid. Thanks guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants