Skip to content

Conversation

@hardbyte
Copy link
Owner

First part of #424

@hardbyte hardbyte requested a review from felixdivo September 30, 2018 10:53
@codecov
Copy link

codecov bot commented Sep 30, 2018

Codecov Report

Merging #440 into develop will increase coverage by 0.01%.
The diff coverage is 85.71%.

@@             Coverage Diff             @@
##           develop     #440      +/-   ##
===========================================
+ Coverage     62.5%   62.51%   +0.01%     
===========================================
  Files           55       55              
  Lines         4657     4661       +4     
===========================================
+ Hits          2911     2914       +3     
- Misses        1746     1747       +1

can/message.py Outdated
@id_type.setter
def id_type(self, value):
# TODO remove in 3.0
# TODO remove in >3.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn’t it be 4.0?

@hardbyte hardbyte merged commit 44d27df into develop Sep 30, 2018
@hardbyte hardbyte deleted the migrate-extended-id branch September 30, 2018 21:50
else:
if not extended_id:
# Passed extended_id=False (default argument is True) so we warn to update
warnings.warn("extended_id is a deprecated parameter, use is_extended_id", DeprecationWarning)
Copy link
Collaborator

Choose a reason for hiding this comment

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

A little late but I don't really understand this. We only want to warn when extended_id=False is used but not extended_id=True?

And I think we should wait with issuing a warning until one or two versions later. Otherwise it won't be possible to support more than the latest python-can version without having thousands of warnings in the log.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or we could print a single warning per interpreter "session".

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.

4 participants