Rewrite ckan.logic.converters.convert_to_extras #2151
Conversation
…oesn't interfere with other extras and complements the related convert_from_extras function. I notice there is no test coverage for these.
|
@ntoll thanks for this. Can you provide a small test similar to these ones that gets fixed by this patch as part of the PR? https://github.com/ckan/ckan/blob/master/ckan/new_tests/logic/test_conversion.py#L17 |
|
Ahh... I see where the tests are now. Why on earth don't they just go in the tests directory..? You're at risk of having "new_tests", "new_tests2", "more_new_tests" and/or "newer_tests" directories... ;-) |
|
OK... not sure how suitable the test I added is. Definitely needs checking given that I've been trying to avoid the internals of CKAN (and I'm therefore not familiar with what's going on or context). Hope this helps. +1 on #1753 ;-) |
| new_pos = 0 | ||
| if extras: | ||
| extras.sort() | ||
| new_pos = extras[-1][-2] + 1 # e.g. ('extras', 5, 'value') |
There was a problem hiding this comment.
You refactored line 16 into new lines 15-18 and TBH I find the original easier to understand.
|
OK, several things going on here :) I've been having a look at this and could reproduce the issue using your extension (you get the exception when you provide more free extras than custom fields, otherwise free extras will make custom fields disappear (as described in #1894)). One thing to note is that on 2.2.1 (the one that your instance targets) you can not mix custom fields that get internally transformed into extras with free form extras when using the core converter. As @davidread mentioned in the previous comment the docs said otherwise but unfortunately this was a a mistake left from bad merging (this is now fixed #2259). AFAICT the issue is fixed when using master or the Looking at patching this in previous versions, I think we agreed that it was too big a change and broke the API, as it required a change in logic to check that the free extras don't clash with the custom ones (for full context see [1]). I'd suggest that you keep using your custom Finally, I'm not saying that this patch is no longer relevant, as you mention it may do things in a safer way ( I need to have another look with more time), but is not directly relevant to fix the original exception you reported. Hope all of this makes sense. [1] https://lists.okfn.org/pipermail/ckan-dev/2014-October/008170.html |
|
Hey hey... great stuff and glad to hear the problem will be fixed in the next release. When is that scheduled to be? |
|
The free-form vs. converted extras problem will be solvable after #3072 is adapted for dataset extras. We'll have a separate namespace available for each plugin so we could easily have these values live side-by-side (and remove the extras table while we're at it) |
|
Looks like this PR is now unnecessary? Suggest we close it. |
This branch introduces a single change:
convert_to_extrasfunction so it behaves in a way that doesn't interfere with other extras and complements the related convert_from_extras function.I notice there is no test coverage for these.
Fixes #2150.