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

replace usage of unicode with unicode_safe #299

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

TomeCirun
Copy link

Fix #298

@TomeCirun
Copy link
Author

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

@smotornyuk
Copy link
Member

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

@TomeCirun
Copy link
Author

TomeCirun commented Sep 17, 2021

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

Thanks

@TomeCirun
Copy link
Author

hey @smotornyuk does my latest commit seems ok ?
Thanks

@smotornyuk
Copy link
Member

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

@wardi
Copy link
Contributor

wardi commented Oct 4, 2021

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

README.md Outdated Show resolved Hide resolved
@TomeCirun
Copy link
Author

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

@TomeCirun TomeCirun requested a review from wardi January 11, 2022 08:33
@ChristianF88
Copy link

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!

@wardi
Copy link
Contributor

wardi commented Mar 27, 2023

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

@ChristianF88
Copy link

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

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.

Replace usage of unicode in schemas and presets
4 participants