Skip to content

Add region splitting #5

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

Merged
merged 7 commits into from
Dec 10, 2024
Merged

Add region splitting #5

merged 7 commits into from
Dec 10, 2024

Conversation

leilafarsani
Copy link
Collaborator

Add region splitting functionality

Changes made:

  • Added IATIActivityRecipientRegion class
  • Implemented region splitting in IATIActivity
  • Added region code support to transaction classes
  • Added tests for region splitting:
    • Basic region splitting
    • Incorrect percentage handling
    • Region normalisation
    • Integration with existing splits

All tests passing.

@leilafarsani leilafarsani requested a review from a user December 6, 2024 10:47
@leilafarsani leilafarsani self-assigned this Dec 6, 2024
@leilafarsani leilafarsani removed the request for review from a user December 6, 2024 10:49
@leilafarsani leilafarsani requested a review from a user December 6, 2024 11:04
@ghost
Copy link

ghost commented Dec 10, 2024

Can we remove the /iati_activity_details_split_by_fields.egg-info directory? This is a build artefact and isn't needed in git. We could add a line to .gitignore about it.

@ghost
Copy link

ghost commented Dec 10, 2024

Similarly the changes to setup.cfg, I'm not sure why they are needed in this PR. They look like packaging things that we can do separately? (I think we can do better values for some of them)

@ghost
Copy link

ghost commented Dec 10, 2024

Readme looks great! Lots of detail. We could move some of it into a proper Sphinx docs site later, as then we can have API reference bits generated automatically from Python and doc strings (also add our nice theme). But it's great to have anything more than the current (ie nothing) for now :-) Some minor comments - we can look at those later in a future PR:

  • Is the Splitting Order actually important? We should get the same value numbers if we split in a different order right? (The results list would be in a different order tho but I don't know if that's important, and I'd be curious to hear any arguments from people if it is important)
  • The "Results will be:" bit in the "Complex Example: Multiple Splits" Usage Examples is great, as then people can read it and see straight away what to expect. The other examples could have that too.
  • The "Output Format" section could note that this is the JSON option and you can also get it in objects. Later when we add other formats we can expand this section.
  • Also reading this made me think of some more tests we could do later, I'll add an issue More Tests: test_some_at_activity_and_some_at_transaction_level.py #7

As for the code and tests, looks great!

The only thing is can we add to the test_split_by_everything test to add a region as well - but then thinking about it, I went hold on a minute, how does that work? Seems like there is cans of worms there? I made an issue and if you agree let's get this PR in and work on those worms later: #6

@leilafarsani leilafarsani merged commit 27ad582 into main Dec 10, 2024
12 checks passed
@leilafarsani leilafarsani deleted the add-region-splitting branch December 10, 2024 12:54
@ghost ghost mentioned this pull request Dec 12, 2024
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.

1 participant