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

Green tests using Py3 - Part 2 #5138

Merged
merged 14 commits into from
Jan 20, 2020
Merged

Conversation

smotornyuk
Copy link
Member

Follows #5134

@smotornyuk smotornyuk removed the WIP label Jan 3, 2020
@amercader amercader self-assigned this Jan 7, 2020
@amercader
Copy link
Member

@smotornyuk can you merge master into this? For some reason Github is not picking up the changes from Part 1

@smotornyuk
Copy link
Member Author

Done

@amercader
Copy link
Member

All good, only one text failure after merging master

@smotornyuk
Copy link
Member Author

Updated - all green again

@amercader amercader merged commit 0e642f7 into ckan:master Jan 20, 2020
@amercader
Copy link
Member

💥 let's get down to business!

Thanks for your work on this @smotornyuk

@frafra
Copy link
Contributor

frafra commented Jan 7, 2022

@smotornyuk Do not use paste.fileapp c1c714d
Could you please comment on that?
I see that it is still used by CKAN 2.9 (dev):

import paste.fileapp

Should that be changed too?

@smotornyuk
Copy link
Member Author

smotornyuk commented Jan 10, 2022

No, you can ignore it. Controllers are not used in 2.9 and just left in codebase for reference

@frafra
Copy link
Contributor

frafra commented Jan 10, 2022

Wouldn't it make sense to remove controllers? It is a confusing reference, since it is code referring to an old version, which is no more supported.

@smotornyuk
Copy link
Member Author

They are already removed in master. In 2.9 we decided not to remove them in order to simplify backports of fixed issues.
For example, I've fixed Flask's view and applied the same fix to the controller(even though it has no effect). When someone prepares a patch release, this person has to cherry-pick my commit, without any extra efforts spent on adapting my fix.

Now we are trying to do backports as soon as the fix got merged into master, so such a measure has no value(and controllers are removed from master anyway). But as we are trying not to change much in the existing branch, nobody has removed controllers:)
If you are working with 2.9 and controllers are bothering you, feel free to remove them. I don't think that somebody will mind merging a PR with such a change

@Zharktas
Copy link
Member

I wouldn't remove controllers from 2.9 as they are still used if you run ckan on python 2 and removing them would essentially drop python2 support.

@frafra
Copy link
Contributor

frafra commented Jan 10, 2022

I wouldn't remove controllers from 2.9 as they are still used if you run ckan on python 2 and removing them would essentially drop python2 support.

Ok, sorry, I missed that :)

@Zharktas
Copy link
Member

Or at least the extensions might rely on those controllers existing, if not in use directly by ckan :)

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.

None yet

4 participants