Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…nto membrane_prototype
|
pre-commit.ci autofix |
hannahbaumann
changed the title
…nto membrane_prototype
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…nto membrane_prototype
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #663 +/- ##
=======================================
Coverage 98.87% 98.87%
=======================================
Files 42 42
Lines 2669 2669
=======================================
Hits 2639 2639
Misses 30 30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hannahbaumann
changed the title
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
No API break detected ✅ |
| try: | ||
| val = int(index, base) | ||
| if val >= 0xA0000: | ||
| val = val - 0xA0000 + 100000 |
There was a problem hiding this comment.
Can we add some ( )to make it more clear the order of operations here?
| return int(index) | ||
| except: | ||
| return int(index, 16) - 0xA0000 + 100000 | ||
| def _parse_atom_index(index: str) -> int: |
There was a problem hiding this comment.
I'd rather this function raise a custom exception that we then catch when we call it instead of a bare except
| for pos in (11, 16, 21, 26): | ||
| try: | ||
| atoms.append(_parse_atom_index(pdb_line[pos : pos + 5])) | ||
| except: |
There was a problem hiding this comment.
We should catch an exception here
| except: | ||
| pass | ||
| self._current_model.connects.append(atoms) | ||
| except: |
There was a problem hiding this comment.
We should catch an exception here
| """ | ||
| index = index.strip() | ||
|
|
||
| # Try decimal, then hex, then Maestro base36 |
There was a problem hiding this comment.
MH: check for a G to see what base we are using, and then use the same base for the entire file and we should be fine
There was a problem hiding this comment.
Just to add a bit more from slack:
It is possible that a base36 might not have a G, in which case the decimal value we convert when we use base16 would be "wrong" but if we use the same base to convert everything into decimal, then things will be internally consistent (for things like CONNECT records).
It also might be nice to have the parsing function take in a base as an argument, so that if a user supplies us a base, we can just use that value.
We can then use another function to guess the base if a user doesn't give us one, then pass that base to the parsing function.
3 participants
Tips
Since this will create a commit, it is best to make this comment when you are finished with your work.
Checklist
newsentryDevelopers certificate of origin