ckanext-scheming icon indicating copy to clipboard operation
ckanext-scheming copied to clipboard

replace usage of unicode with unicode_safe

Open TomeCirun opened this issue 4 years ago • 10 comments

Fix #298

TomeCirun avatar Sep 13 '21 10:09 TomeCirun

hey @wardi what's your opinion over this? Thanks

TomeCirun avatar Sep 15 '21 12:09 TomeCirun

There is a number of places that can be changed as well(even though they do not affect the logic and can be ignored):

smotornyuk avatar Sep 17 '21 08:09 smotornyuk

@smotornyuk thanks for your suggestion and can you please clarify what you mean by or replaced with unicode_safe for backward compatibility

Thanks

TomeCirun avatar Sep 17 '21 09:09 TomeCirun

hey @smotornyuk does my latest commit seems ok ? Thanks

TomeCirun avatar Oct 01 '21 13:10 TomeCirun

or replaced with unicode_safe for backward compatibility

If you just drop it, it's ok for new schemas. But every person who is using unicode validator will get an error. Actually, I like it, because in that way we can be sure that people will replace unicode with unicode_safe.

But, if you want to hide this change (I'll explain, why you may choose this option) and to make your PR compatible with existing schemas(i.e, unicode in schema still can be used), you can write something like

if name == 'unicode':
        name = 'unicode_safe'

does my latest commit seems ok ?

Except for backward compatibility - yes. But you've introduced one issue. the unicode_safe validator was added in CKAN v2.8, while this repo guarantees support for CKAN starting from v2.6. In v2.6 and v2.7 you still have to use unicode validator. This means you have to replace this:

    if name == 'unicode':
        return six.text_type

with something like

    if name == 'unicode' and not ckantoolkit.check_ckan_version('2.8'):
        return six.text_type

smotornyuk avatar Oct 04 '21 15:10 smotornyuk

I'd be happy to add a copy of unicode_safe to ckanext-scheming so it could be used on 2.7. I'm indifferent about 2.6 as that's not an officially supported release anymore

wardi avatar Oct 04 '21 15:10 wardi

Hey Ian, is there anything else that can be done on this PR? Thanks

TomeCirun avatar Oct 27 '21 11:10 TomeCirun

Hi everyone. May I ask, what's gonna happen with this? I am trying to migrate to 2.10 and am experiencing some trouble with this. I guess once more ppl migrate, more will be affected? There are no more logs for the failed tests so I am not sure what the issue is. If it's something I can help fix, I'm up for it!

ChristianF88 avatar Mar 27 '23 12:03 ChristianF88

I don't see a way to re-run tests from the web UI, @TomeCirun if you want to merge again we should be able to see the reason for the test failures, or @ChristianF88 if you like you could create another PR based on this branch

wardi avatar Mar 27 '23 14:03 wardi

I don't see a way to re-run tests from the web UI, @TomeCirun if you want to merge again we should be able to see the reason for the test failures, or @ChristianF88 if you like you could create another PR based on this branch

Hi @TomeCirun, I'm not sure how to interpret the thumbs up. Are you gonna merge again? Cheers

ChristianF88 avatar Mar 28 '23 07:03 ChristianF88