Skip to content

fix android import #1332

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 18, 2018
Merged

fix android import #1332

merged 1 commit into from
Aug 18, 2018

Conversation

akshayaurora
Copy link
Member

No description provided.

@inclement inclement merged commit 7c7d6c8 into master Aug 18, 2018
@akshayaurora akshayaurora deleted the akshayaurora-patch-1 branch August 18, 2018 21:32
@AndreMiras
Copy link
Member

Hi @akshayaurora and @inclement , thank you for spotting the issue and coming up with a fix!
I imagine it's just a reverse of 3d7838f#diff-18498b3e54625d12c3b5f78f5374ec34 that initially introduced this regression, but I feel like we could have improved this pull request a various different ways.

  1. commit body explanation
  2. giving context in the pull request comments
  3. improving the code comment that was here initially
  4. adding the noqa isort ignore
  5. update the "legacy code" that relies on it so it can be dropped
  6. writing a test?

The idea is just to give more context to the future version of ourselves or to new maintainers so we don't make this regression again later.

For 1, 2 and 3, I directly checked on IRC logs to know more.
For 4 I can provide a pull request that ignore the unused import for this file and fix the continuous integration build.
For 5 and 6 I need some investigation because I have no clue how it's currently being using and how to reproduce some of the errors.

I hope all of this make sense.

FWIW a quick'n dirty copy/paste of the IRC conversation:

qua-non
inclement, ping
7:56 pm
https://github.com/kivy/python-for-android/comm...
7:57 pm
most of the existing stuff for android module does not work anymore cause of this
8:12 pm KGB-007
03[kivy]00 03Zen-CODE00 06created00 06* test_docs -00 15 https://git.io/vqY4x 00
8:12 pm
03[kivy]00 06test_docs00 03Zen-CODE00 06committed00 06* doc: fix typo in window docs -00 15https://git.io/fAeS900
8:13 pm
03[kivy]00 03Zen-CODE00 06opened0004 #590000 06* doc: fix typo in window docs -00 15 https://git.io/fAeSQ 00
8:27 pm
03[kivy]00 03KeyWeeUsr00 06merged from Zen-CODE0004 #590000 06* doc: fix typo in window docs -00 15 https://git.io/fAeSQ 00
8:27 pm
03[kivy]00 06master00 03Zen-CODE00 06committed00 06* doc: fix typo in window docs -00 15https://git.io/fAeS900
8:28 pm
03[kivy]00 03KeyWeeUsr00 06deleted00 06* test_docs -00 15 https://git.io/vqY4x 00
9:08 pm
03[kivy]00 03iceblu371000 06opened0004 #590100 06* self.bind(size=self.update) misfiring -00 15 https://git.io/fAeHj 00
10:18 pm tshirtman
matham: sorry, have been mostly away in the few days, to answer your qeustion i've had had weakproxy issues, usually dead references, but i never got the time to investigate root causes, and it was usually in codebases with messy state management, so not sure how much that feedback can help, especially as your concern seem to be more about proxifying more (so solving memory leaks, not dead refs)?
11:26 pm inclement
qua-non: Oh, why does it break it?
11:27 pm qua-non
yeah, all implementation was in _android... without that import none of the implementation works
11:27 pm
probably the reason for many complains of android module stuff not working
11:28 pm inclement
Oh, the android module relied in these otherwise-unused imports?
11:28 pm
That would make sense
11:29 pm
Oh, wait, not it wouldn't
11:29 pm
I don't get it
11:29 pm qua-non
https://github.com/kivy/python-for-android/blob...
11:30 pm
import android returns this
11:30 pm inclement
Right
11:30 pm
Do you want to make a PR to fix it? I can approve it right away
11:30 pm
Or we can do it the other way around
11:30 pm qua-non
this file used to have from android._android import * all implementation that was in android.pyx
11:30 pm inclement
Yep, I get it now
11:31 pm KGB-007
03[python-for-android]00 06akshayaurora-patch-100 03akshayaurora00 06committed00 06* fix android import -00 15https://git.io/fAebg00
11:31 pm
03[python-for-android]00 03akshayaurora00 06created00 06* akshayaurora-patch-1 -00 15 https://git.io/v3PaU 00
11:31 pm
03[python-for-android]00 03akshayaurora00 06opened0004 #133200 06* fix android import -00 15 https://git.io/fAebw 00
11:32 pm qua-non
https://github.com/kivy/python-for-android/pull...
11:32 pm KGB-007
03[python-for-android]00 03inclement00 06merged from akshayaurora0004 #133200 06* fix android import -00 15 https://git.io/fAebw 00
11:32 pm
03[python-for-android]00 06master00 03akshayaurora00 06committed00 06* fix android import -00 15https://git.io/fAebg00
11:32 pm inclement
Done, sorry about that
11:32 pm KGB-007
03[python-for-android]00 03akshayaurora00 06deleted00 06* akshayaurora-patch-1 -00 15 https://git.io/v3PaU 00
11:33 pm qua-non
inclement, no issues, I didn't check if other such issues were raised in any other place...
11:33 pm
came across this when trying to launch service using android.start_service...

@inclement
Copy link
Member

Sorry @AndreMiras, that was sloppy of me. I actually thought it was more obvious than it really is what was wrong. It would have been most appropriate to have added the noqa, in lieu of whatever improvements people might want to make to the android module itself in the future.

@AndreMiras
Copy link
Member

Thanks for the #1340 pull request that fixes to tox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inclement inclement inclement approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@akshayaurora @AndreMiras @inclement