This repository has been archived by the owner on Jan 25, 2024. It is now read-only.
Sort BoardGroup and fix iteration #74
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implementing #37 requires being able to test that Boards in a BoardGroup are sorted by serial number. If serial numbers can differ between test runs, this can lead to inconsistent test results. Therefore we switch from using the `id` (address) of the Board object to using known constant strings.
Previously, boards were sorted by the order in which they were discovered. The discovery order should be considered nondeterministic, since it depends on things like USB addresses for hardware backends and is currently undefined for non-hardware backends. In an effort to make this more useful to a user, boards will now be ordered deterministically by sorting the collection by board serial number. This order is only visible when iterating through the BoardGroup, since #25 removed the ability to index boards by integer indices. Closes #37.
…ependent Previously, the iteration state was contained in the `BoardGroup`, and `BoardGroup.__iter__` simply returned the same `BoardGroup`. This prevented being able to perform multiplie iterations concurrently due to state being incorrectly shared between the corresponding iterators. Closes #38.
|
I'm not sure what to do about these linting errors, pertaining to single-line docstrings being too long. If I attempt to wrap the text onto multiple lines, the linter thinks that I now have a summary line (missing a period, since it is only the first half of a sentence) and a description line (missing a blank line between it and the preceding summary line). Should I just reword the docstring to have a shorter summary sentence? |
|
I'd be happy with either:
|
These were causing linting issues, but cannot be wrapped without the linter instead thinking the docstring contained an unterminated summary and a description not correctly separated from the summary.
sedders123
approved these changes
Feb 13, 2019
| for board in self.board_class.discover(self._backend): | ||
| discovered_boards = self.board_class.discover(self._backend) | ||
| discovered_boards.sort(key=lambda board: board.serial) | ||
| for board in discovered_boards: |
There was a problem hiding this comment.
What is the minimum version of Python we are supporting?
Dictionaries being ordered was introduced to the Python standard in 3.7 and as a side effect to the CPython implementation in 3.6. If we wanted to support Python 3.5 (which is the first to allow syntactical type hints) or non CPython 3.6 we'd have to change self.boards to be an OrderdDict from collections
There was a problem hiding this comment.
Good point, we should really specify this somewhere. I've created #79.
trickeydan
approved these changes
Feb 15, 2019
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #37 and #38. See commit messages for more details.