Feature/refactor package name class #6
Conversation
passes tests
kirillmorozov
left a comment
kirillmorozov
left a comment
There was a problem hiding this comment.
Hi @DavidWLoupe,
I've emailed @AndrewMusser regarding contribution to LPM and one of the things he told me I can help with is the review of this PR.
Beside all my comments I'll also suggest having all new code conform to PEP 8 and have type hints.
| if self._versionText != '': | ||
| return self._versionText | ||
| else: | ||
| return '' No newline at end of file |
There was a problem hiding this comment.
Isn't it the same?
| if self._versionText != '': | |
| return self._versionText | |
| else: | |
| return '' | |
| return self._versionText |
What was your intention behind this check?
| import re | ||
|
|
||
| class PackageNameVersion: | ||
| def __init__(self, input: str): |
There was a problem hiding this comment.
Since you've already started typing your code:
| def __init__(self, input: str): | |
| def __init__(self, input: str) -> None: |
| self._versionText = match.group(4) if match.group(4) is not None else '' | ||
|
|
||
| @property | ||
| def baseName(self): |
There was a problem hiding this comment.
| def baseName(self): | |
| def baseName(self) -> str: |
| return f"{self._baseName}" | ||
|
|
||
| @property | ||
| def fullName(self): |
There was a problem hiding this comment.
| def fullName(self): | |
| def fullName(self) -> str: |
| return f"@loupeteam/{self._baseName}" | ||
|
|
||
| @property | ||
| def version(self): |
There was a problem hiding this comment.
| def version(self): | |
| def version(self) -> str: |
| @@ -0,0 +1,69 @@ | |||
| import pytest | |||
| import sys | |||
| sys.path.append('./src/') | |||
There was a problem hiding this comment.
Manipulation of path smells bad.
What you can do instead is restructure your code to the following:
.
├── docs
├── lpm
│ ├── __init__.py
│ ├── lpm.py
│ └── package.py
├── tests
│ └── test_package.py
├── README.md
└── requirments.txt
This way you could write in your test file
from lpm import packageAnd run tests from the root of the repo.
Extracting tests from src will also make distribution easier because you'll only need to copy src dir and your that will leave alongside won't be distributed.
| from Package import PackageNameVersion | ||
|
|
||
| class Test_PackageNameVersion: | ||
| def test_validInputsNoVersion(self): |
There was a problem hiding this comment.
I like that you use table driven tests but I have a question: why haven't you used pytest parametrisation feature?
| print(colored(f'Could not interpret \"{item}\" as a valid package', 'red')) | ||
|
|
||
| # Return if no valid packages | ||
| if len(packageNameObjectList) == 0: |
There was a problem hiding this comment.
| if len(packageNameObjectList) == 0: | |
| if not packageNameObjectList: |
is more pythonic.
| packagesFullNames = [p.fullName for p in packageNameObjectList] | ||
| packagesBaseNames = [p.baseName for p in packageNameObjectList] | ||
| packagesVersions = [p.version for p in packageNameObjectList] |
There was a problem hiding this comment.
I don't get why have you implemented PackageNameVersion class to have all info regarding a package in one place but then decided to split a list of package into three different lists?
| else: | ||
| packageName = package | ||
| installSource(packageName, '', loupePkg) | ||
| for (packageFullName, packageBaseName, version) in zip(packagesFullNames, packagesBaseNames, packagesVersions): |
There was a problem hiding this comment.
Here you zip them together to iterate over three lists but if you would have kept it in a single list it would be as easy as:
for package in packages:
installSource(package.fullName, package.version, loupePkg)or with further refactoring to installSource function even simpler
for package in packages:
installSource(package)
What
Adds a class for collecting a package name and providing an interface for getting it in different formats. The three class properties that are supported are:
@loupeteam/atn)atn)3.1.0)Why
Previously there were a variety of methods strewn about for adding or splitting up names and versions. This seeks to unify the issue by channeling it through an object interface