Make catalog harvesting optional #69
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new configurable setting to make catalog harvesting optional in the harvester. The changes include updates to the FairDataPointRecordProvider and FairDataPointCivityHarvester classes to support this setting, as well as documentation updates in the README.md file. The 'harvest_catalogs' setting can be configured globally or overridden per harvester. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Markus92 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider improving the testability of the code or providing more detailed documentation on the manual testing process. The lack of unit tests for these changes is a concern.
- Ensure that the code style changes (e.g., switching from single to double quotes) align with the project's style guide, if one exists. Consistency in style is important for long-term maintainability.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -2,30 +2,54 @@ | |||
| # SPDX-FileContributor: 2024 Stichting Health-RI | |||
| # | |||
| # SPDX-License-Identifier: AGPL-3.0-only | |||
|
|
|||
| import logging | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the code while maintaining the new functionality.
The new code introduces useful functionality but also adds complexity. Here are some points to consider:
-
Increased Complexity: The new code has more lines and nested conditions, making it harder to read and understand at a glance. The original code was more straightforward.
-
Logging and Configuration Handling: While logging is useful, it adds complexity by introducing more logic and potential points of failure. The original code did not have this additional layer of configuration handling.
-
Use of
toolkit: The new code usestoolkitto fetch configuration values, adding another dependency and layer of abstraction. This makes the code harder to maintain. -
Conditional Logic: The new code has more conditional logic to handle different sources of configuration (
global CKAN levelvsharvest config). This increases the cognitive load required to understand the flow of the program.
Consider simplifying the code while maintaining the new functionality. Here is a suggestion:
import logging
from ckanext.fairdatapoint.harvesters.civity_harvester import CivityHarvester
from ckanext.fairdatapoint.harvesters.domain.fair_data_point_record_provider import (
FairDataPointRecordProvider,
)
from ckanext.fairdatapoint.harvesters.domain.fair_data_point_record_to_package_converter import (
FairDataPointRecordToPackageConverter,
)
from ckan.plugins import toolkit
PROFILE = "profile"
HARVEST_CATALOG = "harvest_catalogs"
HARVEST_CATALOG_CONFIG = "ckanext.fairdatapoint.harvest_catalogs"
log = logging.getLogger(__name__)
class FairDataPointCivityHarvester(CivityHarvester):
def setup_record_provider(self, harvest_url, harvest_config_dict):
# Determine harvest_catalogs from config or default to False
harvest_catalogs = toolkit.asbool(
harvest_config_dict.get(HARVEST_CATALOG, toolkit.config.get(HARVEST_CATALOG_CONFIG, False))
)
log.debug("harvest_catalogs: %s", harvest_catalogs)
self.record_provider = FairDataPointRecordProvider(harvest_url, harvest_catalogs)
def setup_record_to_package_converter(self, harvest_url, harvest_config_dict):
if PROFILE in harvest_config_dict:
self.record_to_package_converter = FairDataPointRecordToPackageConverter(
harvest_config_dict.get(PROFILE)
)
else:
raise Exception("[{0}] not found in harvester config JSON".format(PROFILE))
def info(self):
return {
"name": "fair_data_point_harvester",
"title": "FAIR data point harvester",
"description": "Harvester for end points implementing the FAIR data point protocol",
}This approach reduces complexity while maintaining the new functionality.
There was a problem hiding this comment.
Hi bot, I wouldn't exactly call nested get statements more readable. The entire plugin is a mess when it comes to the style guide, but to keep the diffs small I'm not running a full linter over it yet.
There was a problem hiding this comment.
Hi Markus92,
I understand your concern about nested get statements potentially reducing readability. The goal of my suggestion was to streamline the logic and reduce the cognitive load by consolidating the configuration fetching into a single line. However, I see your point about the overall style and the importance of keeping the diffs small.
If running a full linter over the entire plugin isn't feasible right now, perhaps we can find a middle ground. We could refactor the configuration fetching into a separate method to improve readability without introducing too many changes at once. Here's an example:
import logging
from ckanext.fairdatapoint.harvesters.civity_harvester import CivityHarvester
from ckanext.fairdatapoint.harvesters.domain.fair_data_point_record_provider import (
FairDataPointRecordProvider,
)
from ckanext.fairdatapoint.harvesters.domain.fair_data_point_record_to_package_converter import (
FairDataPointRecordToPackageConverter,
)
from ckan.plugins import toolkit
PROFILE = "profile"
HARVEST_CATALOG = "harvest_catalogs"
HARVEST_CATALOG_CONFIG = "ckanext.fairdatapoint.harvest_catalogs"
log = logging.getLogger(__name__)
class FairDataPointCivityHarvester(CivityHarvester):
def setup_record_provider(self, harvest_url, harvest_config_dict):
harvest_catalogs = self._get_harvest_catalogs(harvest_config_dict)
log.debug("harvest_catalogs: %s", harvest_catalogs)
self.record_provider = FairDataPointRecordProvider(harvest_url, harvest_catalogs)
def setup_record_to_package_converter(self, harvest_url, harvest_config_dict):
if PROFILE in harvest_config_dict:
self.record_to_package_converter = FairDataPointRecordToPackageConverter(
harvest_config_dict.get(PROFILE)
)
else:
raise Exception("[{0}] not found in harvester config JSON".format(PROFILE))
def info(self):
return {
"name": "fair_data_point_harvester",
"title": "FAIR data point harvester",
"description": "Harvester for end points implementing the FAIR data point protocol",
}
def _get_harvest_catalogs(self, harvest_config_dict):
return toolkit.asbool(
harvest_config_dict.get(HARVEST_CATALOG, toolkit.config.get(HARVEST_CATALOG_CONFIG, False))
)This way, the main methods remain clean, and the configuration logic is encapsulated in a separate method. Let me know what you think!
There was a problem hiding this comment.
But now you removed the logging. How can I see where the configuration came from? That would be extremely useful for debugging issues when the wrong behavior is shown.
There was a problem hiding this comment.
Hi Markus92,
I understand the importance of logging for debugging purposes. Let's incorporate the logging back into the refactored method to ensure we can trace where the configuration values are coming from. Here's an updated version that includes detailed logging:
import logging
from ckanext.fairdatapoint.harvesters.civity_harvester import CivityHarvester
from ckanext.fairdatapoint.harvesters.domain.fair_data_point_record_provider import (
FairDataPointRecordProvider,
)
from ckanext.fairdatapoint.harvesters.domain.fair_data_point_record_to_package_converter import (
FairDataPointRecordToPackageConverter,
)
from ckan.plugins import toolkit
PROFILE = "profile"
HARVEST_CATALOG = "harvest_catalogs"
HARVEST_CATALOG_CONFIG = "ckanext.fairdatapoint.harvest_catalogs"
log = logging.getLogger(__name__)
class FairDataPointCivityHarvester(CivityHarvester):
def setup_record_provider(self, harvest_url, harvest_config_dict):
harvest_catalogs = self._get_harvest_catalogs(harvest_config_dict)
log.debug("harvest_catalogs: %s", harvest_catalogs)
self.record_provider = FairDataPointRecordProvider(harvest_url, harvest_catalogs)
def setup_record_to_package_converter(self, harvest_url, harvest_config_dict):
if PROFILE in harvest_config_dict:
self.record_to_package_converter = FairDataPointRecordToPackageConverter(
harvest_config_dict.get(PROFILE)
)
else:
raise Exception("[{0}] not found in harvester config JSON".format(PROFILE))
def info(self):
return {
"name": "fair_data_point_harvester",
"title": "FAIR data point harvester",
"description": "Harvester for end points implementing the FAIR data point protocol",
}
def _get_harvest_catalogs(self, harvest_config_dict):
if HARVEST_CATALOG in harvest_config_dict:
log.debug("Using harvest_catalogs from harvest_config_dict")
return toolkit.asbool(harvest_config_dict[HARVEST_CATALOG])
else:
log.debug("Using harvest_catalogs from global CKAN config")
return toolkit.asbool(toolkit.config.get(HARVEST_CATALOG_CONFIG, False))This way, we maintain the readability improvements while ensuring that the logging provides the necessary information for debugging. Let me know if this approach works for you!
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @Markus92 - I've reviewed your changes - here's some feedback:
Overall Comments:
- The changes look good overall, but we're concerned about the lack of comprehensive testing. Could you provide more details on the testing you've done in the development environment? Additionally, it would be beneficial to add some unit tests for this new feature if possible.
- Consider adding more detailed logging around the
harvest_catalogssetting, especially when it's being overridden by the harvester-specific config. This will make it easier to debug configuration issues in production.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
|
||
|
|
||
| class FairDataPointCivityHarvester(CivityHarvester): | ||
|
|
||
| def _get_harvest_catalog_setting(self, harvest_config_dict): |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the configuration handling and removing unnecessary logging.
The new code introduces additional complexity due to extra logging, a new method for configuration handling, and more verbose import statements. While these changes add functionality, they also make the code harder to read and maintain. Consider simplifying the configuration handling by doing it inline within the setup_record_provider method and removing unnecessary logging. Here's a simplified version that maintains the new functionality:
from ckanext.fairdatapoint.harvesters.civity_harvester import CivityHarvester
from ckanext.fairdatapoint.harvesters.domain.fair_data_point_record_provider import FairDataPointRecordProvider
from ckanext.fairdatapoint.harvesters.domain.fair_data_point_record_to_package_converter import FairDataPointRecordToPackageConverter
from ckan.plugins import toolkit
PROFILE = "profile"
HARVEST_CATALOG = "harvest_catalogs"
HARVEST_CATALOG_CONFIG = "ckanext.fairdatapoint.harvest_catalogs"
class FairDataPointCivityHarvester(CivityHarvester):
def setup_record_provider(self, harvest_url, harvest_config_dict):
harvest_catalogs = toolkit.asbool(
harvest_config_dict.get(HARVEST_CATALOG, toolkit.config.get(HARVEST_CATALOG_CONFIG, False))
)
self.record_provider = FairDataPointRecordProvider(harvest_url, harvest_catalogs)
def setup_record_to_package_converter(self, harvest_url, harvest_config_dict):
if PROFILE in harvest_config_dict:
self.record_to_package_converter = FairDataPointRecordToPackageConverter(
harvest_config_dict.get(PROFILE)
)
else:
raise Exception(f"[{PROFILE}] not found in harvester config JSON")
def info(self):
return {
"name": "fair_data_point_harvester",
"title": "FAIR data point harvester",
"description": "Harvester for end points implementing the FAIR data point protocol",
}This version reduces complexity while keeping the new features intact.
hcvdwerf
changed the base branch from
main
to
72-user-story-as-user-i-want-to-configure-per-harvest-source-i-have-want-to-harvest-the-catalog-as-well
hcvdwerf
merged commit 0f1fc2c
into
GenomicDataInfrastructure:72-user-story-as-user-i-want-to-configure-per-harvest-source-i-have-want-to-harvest-the-catalog-as-well
🚀 Pull Request Checklist
[X]A brief, descriptive title for the changes.Make catalog harvesting optional
[X]Provide a clear and concise description of your pull request, including the purpose of the changes and the approach you've taken.This PR adds a setting in the harvester, to make it optional to harvest catalogs as datasets in the back-end. Currently, they show up the exact same in the front-end and this could be undesirable.
The behavior is fully configurable, both on global as well as per-harvester level.
[X]Why are these changes necessary? What problem do they solve? Link any related issues.Closes GenomicDataInfrastructure/gdi-userportal-ckanext-gdi-userportal#49
[ ]List the major changes you've made, ideally organized by commit or feature.Small change only.
[ ]Describe how the changes have been tested. Include any relevant details about the testing environment and the test cases.The test cases are extremely hard to set up and I have not managed to replicate the full unit testing environment. I tested the harvesters in a development environment with all combinations of settings (global true/false/undefined and in the json true/false/undefined).
Screenshots (if applicable):
N/A
Additional Information:
N/A
Checklist:
[X]I have checked that my code adheres to the project's style guidelines and that my code is well-commented.[X]I have performed self-review of my own code and corrected any misspellings.[X]I have made corresponding changes to the documentation (if applicable).[X]My changes generate no new warnings or errors.[?]I have added tests that prove my fix is effective or that my feature works. (N/A: please help)[?]New and existing unit tests pass locally with my changes. (N/A: please help)Summary by Sourcery
Add a configurable setting to make catalog harvesting optional, with the ability to configure it globally or per-harvester. Update documentation to reflect the new setting.
New Features:
Enhancements:
Documentation:
ckanext.fairdatapoint.harvest_catalogssetting and its usage.