Bug 656968
| Summary: | duplicate code | ||
|---|---|---|---|
| Product: | Red Hat Enterprise Linux 6 | Reporter: | Miroslav Suchý <msuchy> |
| Component: | subscription-manager | Assignee: | Bryan Kearney <bkearney> |
| Status: | CLOSED WONTFIX | QA Contact: | Bryan Kearney <bkearney> |
| Severity: | medium | Docs Contact: | |
| Priority: | low | ||
| Version: | 6.1 | CC: | bkearney, jmolet, jsefler, shaines |
| Target Milestone: | rc | ||
| Target Release: | --- | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2011-07-28 18:05:34 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Bug Depends On: | |||
| Bug Blocks: | 682238 | ||
pylint Main offenders are removed. There is a couple of import dups, but I am not going to worry about that. Fixed in commit: 7828287a80aa9896ff6cf72b27d3c9327c2584bd in master. in subscription-manager git: find src -name '*.py'|xargs pylint 4.5 % of code is still duplicate and e.g. class factlib.FactLib and repolib.RepoLib and 100% identical. And it is 75% identical to certlib.CertLib another example is: ./src/subscription_manager/managercli.py function CliCommand._request_validity_check should use utils.get_dbus_iface instead of its 5 first lines. Additionally to comment #7 - are you sure you committed your work? Because such commit does not exist: [msuchy@dri/~/rhn/subscription-manager{master}]$ git pull --rebase Current branch master is up to date. [msuchy@dri/~/rhn/subscription-manager{master}]$ git show 7828287a80aa9896ff6cf72b27d3c9327c2584bd fatal: bad object 7828287a80aa9896ff6cf72b27d3c9327c2584bd Please look at: http://git.fedorahosted.org/git/?p=subscription-manager.git find src -name '*.py'|xargs pylint gives me: Duplication ----------- +-------------------------+------+---------+-----------+ | |now |previous |difference | +=========================+======+=========+===========+ |nb duplicated lines |368 |368 |= | +-------------------------+------+---------+-----------+ |percent duplicated lines |4.336 |4.336 |= | +-------------------------+------+---------+-----------+ Quality Engineering Management has reviewed and declined this request. You may appeal this decision by reopening this request. |
R: 1: Similar lines in 3 files ==certlib:52 ==factlib:29 ==repolib:30 def __init__(self, lock=ActionLock()): self.lock = lock def update(self): lock = self.lock lock.acquire() try: action = UpdateAction() return action.perform() finally: lock.release() Class CertLib, FactLib, RepoLib are identical. Use only one. Or create one parent and those 3 will inherit from it. R: 1: Similar lines in 2 files ==managerlib:508 ==managerlib_async:58 def filter_pools(self, compatible, overlapping, uninstalled, text): """ Return a list of pool hashes, filtered according to the given options. This method does not actually hit the server, filtering is done in memory. """ pools = self.incompatible_pools.values() if compatible: pools = self.compatible_pools.values() pool_filter = PoolFilter() # Filter out products that are not installed if necessary: if uninstalled: pools = pool_filter.filter_out_installed(pools) else: pools = pool_filter.filter_out_uninstalled(pools) # Do nothing if set to None: if overlapping: pools = pool_filter.filter_out_non_overlapping(pools) elif overlapping == False: pools = pool_filter.filter_out_overlapping(pools) # Filter by product name if necessary: if text: pools = pool_filter.filter_product_name(pools, text) return pools def merge_pools(self, compatible=True, overlapping=None, uninstalled=False, text=None): """ Return a merged view of pools filtered according to the given options. Pools for the same product will be merged into a MergedPool object. Overlapping filter by default is None, meaning the pools will not be filtered at all. Use True to filter out pools which do not overlap, or False to filter out pools which do. """ pools = self.filter_pools(compatible, overlapping, uninstalled, text) merged_pools = merge_pools(pools) return merged_pools R: 1: Similar lines in 2 files ==managerlib:487 ==managerlib_async:36 self.all_pools = {} self.compatible_pools = {} log.debug("Refreshing pools from server...") for pool in list_pools(self.backend.uep, self.consumer.uuid, self.facts, active_on=active_on): self.compatible_pools[pool['id']] = pool self.all_pools[pool['id']] = pool # Filter the list of all pools, removing those we know are compatible. # Sadly this currently requires a second query to the server. self.incompatible_pools = {} for pool in list_pools(self.backend.uep, self.consumer.uuid, self.facts, all=True, active_on=active_on): if not pool['id'] in self.compatible_pools: self.incompatible_pools[pool['id']] = pool self.all_pools[pool['id']] = pool log.debug("found %s pools:" % len(self.all_pools)) log.debug(" %s compatible" % len(self.compatible_pools)) log.debug(" %s incompatible" % len(self.incompatible_pools)) There is no need to duplicate so much code of PoolStash class. Create PoolStashBase with common code and in both module change only that part which behave differently.