Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor to class #30

Merged
merged 12 commits into from
Jan 27, 2024
Merged

Refactor to class #30

merged 12 commits into from
Jan 27, 2024

Conversation

rshewitt
Copy link
Contributor

@rshewitt rshewitt commented Jan 18, 2024

Pull Request

Related to #4577

About

  • refactoring harvester functionality and tests as classes
  • i'm using the @property decorator to make some fields read-only. it serves a similar benefit to const in node.
    • i'm not using the decorator in some cases where the dtype is a dictionary because we'd need to create a custom dict class and overload the __setitem__ method. is it better to be consistent?
    • the non-dict fields which will be assigned after initialization have setter methods configured to them with basic type checking
  • a majority of tests have been removed. this is for a couple reasons
    • the number of tests aren't needed ( e.g. validation and extract )
    • some tests weren't added back into this refactor in an effort to get this codebase available to @btylerburton more quickly so he can perform his load test.
    • furthermore, I simply wanted to get the coverage reasonably high while also minimizing our codebase as a sort-of fresh start.
  • nothing is being done with the transform ( i.e no tests or functionality ). we're not transforming anything in our load test because our transformation service isn't ready although progress is actively being made so maybe we include it?
  • RemoteCKAN is created outside of any dataclass to avoid any potential serialization issue when/if we decide to store an intermediate HarvestSource instance in somewhere like s3.
  • Record instantiation occurs primarily in harvest_to_id_hash but only for records that need to be created or updated NOT deleted. for the sake of record-keeping ( no pun-intended ) I instantiated Record for deleted records immediately before deleting them on ckan.
  • there's purposefully no try/excepts ( excluding the validation function because i copy/pasted that ) because we don't have a concrete idea of our exception handling.

PR TASKS

  • The actual code changes.
  • Tests written and passed.
  • Any changes to docs?
  • Bumped version number in pyproject.toml (also checked on PyPi).

Copy link

github-actions bot commented Jan 18, 2024

Coverage

Coverage Report
FileStmtsMissCoverMissing
harvester
   __init__.py00100% 
   harvest.py3485454 84%
   utils.py100100% 
TOTAL3585485% 

Tests Skipped Failures Errors Time
7 0 💤 0 ❌ 0 🔥 0.963s ⏱️

@rshewitt rshewitt marked this pull request as draft January 18, 2024 01:35
@jbrown-xentity
Copy link
Contributor

As someone who's been tracking this progress all week, my suggestion for reviewing this is to look at the code state at the latest commit, not the changes. GitHub doesn't know how to show this many changes, it's messy. But the code is much cleaner.

@rshewitt rshewitt marked this pull request as ready for review January 18, 2024 17:39
@rshewitt rshewitt marked this pull request as draft January 18, 2024 18:17
harvester/harvest.py Show resolved Hide resolved
harvester/harvest.py Outdated Show resolved Hide resolved
harvester/harvest.py Outdated Show resolved Hide resolved
harvester/harvest.py Show resolved Hide resolved
@rshewitt rshewitt marked this pull request as ready for review January 26, 2024 22:15
Copy link
Contributor

@jbrown-xentity jbrown-xentity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me, reviewed in person.

@rshewitt rshewitt merged commit 87b8798 into main Jan 27, 2024
1 check passed
@rshewitt rshewitt deleted the refactor-to-class branch January 27, 2024 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants