Skip to content

Enforce pylint violation when using old event tracker. - #6080

Merged
benpatterson merged 1 commit into
masterfrom
benp/find-deprecated-tracker
Dec 3, 2014
Merged

Enforce pylint violation when using old event tracker.#6080
benpatterson merged 1 commit into
masterfrom
benp/find-deprecated-tracker

Conversation

@benpatterson

Copy link
Copy Markdown
Contributor

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

@benpatterson

Copy link
Copy Markdown
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
Copy Markdown
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

sarina commented Dec 1, 2014

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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

sarina commented Dec 2, 2014

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

sarina commented Dec 3, 2014

Copy link
Copy Markdown
Contributor

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

mulby commented Dec 3, 2014

Copy link
Copy Markdown
Contributor

Found the ticket

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

@benpatterson

Copy link
Copy Markdown
Contributor Author

Solid. Thanks guys.

benpatterson pushed a commit that referenced this pull request Dec 3, 2014
Enforce pylint violation when using old event tracker.
@benpatterson
benpatterson merged commit c09f0ea into master Dec 3, 2014
@benpatterson
benpatterson deleted the benp/find-deprecated-tracker branch December 3, 2014 19:04
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