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

Feature ch-stats as a separate folder #317

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

adrienmellot
Copy link
Contributor

@adrienmellot adrienmellot commented Apr 8, 2024

Fixes #313
Partial fix for #315

Co-authored by: Francesco Sanvito sanvitofrancesco@gmail.com

Checklist

Any checks which are not relevant to the PR can be pre-checked by the PR creator. All others should be checked by the reviewer. You can add extra checklist items here if required by the PR.

  • CHANGELOG updated
  • Minimal workflow tests pass
  • Tests added to cover contribution
  • Documentation updated
  • Configuration schema updated

@sjpfenninger
Copy link
Member

I've added a fix for the linux verison conflicts issue. The PR now builds for me successfully on linux-x86. @brynpickering @adrienmellot can you maybe do a check if this doesn't break on macOS in turn?

@timtroendle
Copy link
Member

Thanks for the PR. This is a large PR that doesn't seem to add any feature as far as I can tell. Would it be possible to refactor this change to something that actually adds a feature or fixes a bug? Why not proving a fix for #291? It will be really difficult to review this without a clear purpose.

I understand this may be the wrong time and it may not be reasonable to refactor it now. In that case, let me know and I will start trying to review.

@adrienmellot
Copy link
Contributor Author

Fair point. Initially the point for this PR was to move towards #315 gradually, by first including ch-stats and then including Eurostat processing separately, before finally merging both with the actual workflow. In the meantime other commits have piled up to solve environment issues on linux. As you point out this doesn't solve anything yet, and in the meantime #291 was solved separately.

@brynpickering
Copy link
Member

@timtroendle: as @adrienmellot mentioned, this is part of a wider aim to split out data pre-processing based on source rather than sector. If we want (relatively) small PRs, we cannot expect every one to fit neatly into a feature or bug fix - some are simply refactoring or a step in the right direction towards an end goal. In this case, @adrienmellot has linked to an issue that describes the purpose of this PR and I have updated the comment to link to the meta issue.

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.

Separate ch-stats downloading and processing
4 participants