Skip to content

Conversation

@larsoner
Copy link
Member

Closes #7635

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

Questions:

  1. Do you think it's worth it to start using abstract base class Collection in type checking?
  2. Any parts of the codebase that would benefit from namedtuple?

@drammock drammock changed the title MRG, MAINT: Support only 3.5+ MRG, MAINT: Support only 3.6+ Apr 20, 2020
@cbrnr
Copy link
Contributor

cbrnr commented Apr 20, 2020

👍 for type-checking against an ABC.

@larsoner
Copy link
Member Author

Do you think it's worth it to start using abstract base class Collection in type checking?

Maybe but it will probably need more testing, so I'd rather let someone else tackle it in another PR

@agramfort
Copy link
Member

would someone with python 2.5 still be able to use mne 0.21 and have silent bugs?

@larsoner
Copy link
Member Author

would someone with python 2.5 still be able to use mne 0.21 and have silent bugs?

No because:

  1. imports like from collections.abc will fail, and
  2. we won't even allow them to install with our new python_requires line in setup.py

@agramfort
Copy link
Member

agramfort commented Apr 20, 2020 via email

@larsoner
Copy link
Member Author

Any parts of the codebase that would benefit from namedtuple?

In principle anywhere we return a tuple could probably benefit from it, I guess

@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #7638 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7638      +/-   ##
==========================================
- Coverage   90.14%   90.08%   -0.07%     
==========================================
  Files         454      454              
  Lines       83929    83583     -346     
  Branches    13324    13230      -94     
==========================================
- Hits        75661    75298     -363     
- Misses       5406     5423      +17     
  Partials     2862     2862              

@larsoner
Copy link
Member Author

Rebased, okay to merge once green @agramfort ?

@agramfort
Copy link
Member

+1 for MRG when CIs are happy

@larsoner larsoner merged commit 24eae37 into mne-tools:master Apr 21, 2020
@larsoner larsoner deleted the 3.6 branch April 21, 2020 16:26
stlukyanenko pushed a commit to stlukyanenko/mne-python that referenced this pull request Apr 21, 2020
'CP4', 'PO8', 'P8', 'P6', 'CP6', 'PO10', 'TP10', 'TP8', 'FT10', 'T8',
'C6', 'FT8'
]
assert set(EXPECTED_CH_NAMES) == set(EXPECTED_CH_NAMES_OLD)
Copy link
Member

Choose a reason for hiding this comment

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

@larsoner why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of being alphabetical it now should be the order on disk. I figured it shouldn't affect being able to actually use it with our montage functions...

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.

MAINT: Require 3.6+

5 participants