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.

Sign up for GitHub

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

Jump to bottom

add a deprecated notice to 'SqlReader' for users #263

Closed
wants to merge 6 commits into from
Closed

add a deprecated notice to 'SqlReader' for users #263

wants to merge 6 commits into from

Conversation

felixdivo
Copy link
Collaborator

I changed the structure of setup.py a little bit and added the Deprecated library as a dependency, as suggested by Brian. Then I added the @deprecated decorator to mark SqlReader as - you guessed it - deprecated.

@felixdivo felixdivo added this to the 2.2 Release milestone Feb 22, 2018
@felixdivo felixdivo self-assigned this Feb 22, 2018
@felixdivo
Copy link
Collaborator Author

Oh wow, the Deprecated library is strange. Anyways, it seems like we forgot to include SqlReader in can/__init__.py and can/io/__init__.py, so it already broke comparability in the last Release. That makes all of this obsolete, right?

hardbyte
hardbyte reviewed Feb 23, 2018
View reviewed changes
can/io/sqlite.py
# TODO remove in later releases?
SqlReader = SqliteReader
# SqliteReader is the newer name
SqliteReader = SqlReader
Copy link
Owner

Choose a reason for hiding this comment

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

Won't the SqliteReader also be marked as deprecated with this approach?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes yes, it all doesn't work like I intended. But de we really need this deprecation at all? We already broke compatibility.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm I didn't realize we'd already broken compatibility - in that case no we don't really need this deprecation warning. Lets just close this PR and remove the SqlReader/SqliteReader alias.

@felixdivo
Copy link
Collaborator Author

Yeah, I will do that.

@felixdivo felixdivo closed this in e4569c4 Feb 24, 2018
@felixdivo felixdivo deleted the deprecate-notice branch February 24, 2018 00:19
boris-wenzlaff pushed a commit to boris-wenzlaff/python-can that referenced this pull request Mar 23, 2018
@felixdivo
completely remove SqlReader. closes hardbyte#263
be4ea28
@hardbyte hardbyte mentioned this pull request Jun 30, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hardbyte hardbyte hardbyte left review comments

Assignees

@felixdivo felixdivo

Labels
api minor
Projects
None yet
Milestone
2.2 Release
Development

Successfully merging this pull request may close these issues.
2 participants
@felixdivo @hardbyte