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

Introduce Table.remove_data() #159

Open
wants to merge 1 commit into
base: enh/introduce-indexset-remove-data
Choose a base branch
from

Conversation

glatterf42
Copy link
Member

As announced in #158, this PR adds Table.remove_data() (and fixes some type: ignore comments).

Two things to note here, mainly:

  • It would be ideal to find a way to skip the data validation when removing data. As the code is written now, table.data = ... triggers the data validation, but when we're removing data, we don't actually need to validate the data remaining in the table as that has been validated when we added it previously.
    Unfortunately, sqlalchemy's docs don't hint at any possibility to skip the validation check when using @validates as we do. They do state that removals from a collection generally don't trigger this, but we don't have a proper collection, just a dict data attribute. So this would be a benefit of normalizing our data model further, since table.data would then be a proper collection.
    In the meantime, though, I was wondering whether we need @validates at all. We do need to validate data, but we don't expose table.data in the DB layer, so there's no chance of a user accidentally setting this to some invalid data. Thus, I'm not sure we benefit all that much from the @validates functionality of automatically checking .data each time it is set; we could simply make sure that add_data() always calls our validation function manually -- and conversely, that remove_data() never does that.
    What do you think?
  • I'm not entirely sure why remove_data() doesn't seem to need a self.session.commit(), while add_data() does need it to pass the tests. I'm not sure we need to worry about this, either, though, as I'm happy as long as the test work as intended.

@glatterf42 glatterf42 added the enhancement New feature or request label Feb 4, 2025
@glatterf42 glatterf42 requested a review from meksor February 4, 2025 08:23
@glatterf42 glatterf42 self-assigned this Feb 4, 2025
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.5%. Comparing base (061a25d) to head (8eec6db).

Additional details and impacted files
@@                        Coverage Diff                         @@
##           enh/introduce-indexset-remove-data    #159   +/-   ##
==================================================================
  Coverage                                88.4%   88.5%           
==================================================================
  Files                                     223     223           
  Lines                                    8055    8083   +28     
==================================================================
+ Hits                                     7126    7155   +29     
+ Misses                                    929     928    -1     
Files with missing lines Coverage Δ
ixmp4/core/optimization/table.py 92.7% <100.0%> (+0.3%) ⬆️
ixmp4/data/abstract/optimization/table.py 97.0% <100.0%> (+<0.1%) ⬆️
ixmp4/data/api/optimization/equation.py 94.2% <100.0%> (ø)
ixmp4/data/api/optimization/table.py 94.6% <100.0%> (+0.6%) ⬆️
ixmp4/data/api/optimization/variable.py 94.2% <100.0%> (ø)
ixmp4/data/db/optimization/table/repository.py 98.6% <100.0%> (+0.3%) ⬆️
ixmp4/server/rest/optimization/table.py 97.2% <100.0%> (+0.3%) ⬆️

... and 1 file with indirect coverage changes

@glatterf42 glatterf42 force-pushed the enh/introduce-indexset-remove-data branch from 9145994 to 061a25d Compare February 5, 2025 09:37
@glatterf42 glatterf42 force-pushed the enh/introduce-table-remove-data branch from b942822 to 8eec6db Compare February 5, 2025 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant