Implement import mappings for descriptions#614
Merged
Conversation
Changed the logic on how entity (class) mappings work since adding descriptions to the previous system turned out to be quite hacky. We now collect the imported data into 'records' (EntityClassRecord, EntityRecord) which we process into import data for import_functions at the end of get_mapped_data(). Also, removed the raise KeyError()/raise KeyFix() system that didn't make sense to do on every single imported row. We should have some dedicated machinery that validates the mappings without affecting the actual import process instead. In any case, the changes seem to improve the performance by a lot: unit tests on my system execute in 2:45 with these changes while before this commit, they took 3:30. Also a real-life Importer execution time is down to 5s from 9s. Re spine-tools/Spine-Toolbox#1892
- ParameterValueListMapping cannot unconditionally require ParameterDefinitionMapping as parent. A smarter validation mechanism has been implemented to fix this. - We should let KeyErrors in ImportMapping.import_row() to propagate. They are now always logic errors as we do not use KeyErrors for mapping validation anymore. - Fixed a bug in EntityMapping where ND entities from the first entity class encountered on a table were imported but nothing else. Re spine-tools/Spine-Toolbox#1892
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #614 +/- ##
==========================================
- Coverage 86.05% 86.05% -0.01%
==========================================
Files 83 83
Lines 11225 11433 +208
Branches 1627 1635 +8
==========================================
+ Hits 9660 9839 +179
- Misses 1210 1241 +31
+ Partials 355 353 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
New chardet breaks our unit tests; however, it seems to be much more performant (it's a complete rewrite after all) and capable than the previous versions. So, let's switch to the latest and creates once and for all.
soininen
force-pushed
the
toolbox_1892_import_descriptions
branch
from
7103200 to
e23dcc7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements import mappings for entity class, entity, parameter value, scenario and alternative descriptions.
Re spine-tools/Spine-Toolbox#1892
Checklist before merging