kivy / python-for-android

Turn your Python application into an Android APK
https://python-for-android.readthedocs.io
MIT License
8.33k stars 1.85k forks source link

Sort values in log under `get_recipe_order_and_bootstrap` and `get_recipe_order_and_bootstrap` functions. #2930

Open UriyaHarpeness opened 10 months ago

UriyaHarpeness commented 10 months ago

This is a matter of preference.

When running consecutive builds, I myself prefer the output to be as similar as possible, thriving for 100% match - this becomes more useful when looking for difference from a former build, and reducing the "noise". Python sets are a very common pitfall for this, as the order is not guaranteed, so for running the same build twice you would get the log Will compile for the following archs: arm64-v8a, armeabi-v7a and right after it Will compile for the following archs: armeabi-v7a, arm64-v8a. Same "issue" with the log Dist will also contain modules (pillow~=10.1, kivymd~=1.1, kivy~=2.2) installed from pip vs Dist will also contain modules (kivy~=2.2, pillow~=10.1, kivymd~=1.1) installed from pip.

The fix is as easy as swapping the list to sorted, would gladly open the PR myself, but first would like to get some feedback as for whether it matters to anyone besides me, or this issue is a nuisance.

Side note: There can be more places with the same "issue", if this issue is accepted, possibly more PRs would follow.

https://github.com/kivy/python-for-android/blob/436d5a93282dcfcc7027d19d70963c6773544434/pythonforandroid/build.py#L396-L407 https://github.com/kivy/python-for-android/blob/436d5a93282dcfcc7027d19d70963c6773544434/pythonforandroid/graph.py#L340

Julian-O commented 10 months ago

as for whether it matters to anyone besides me, or this issue is a nuisance

How might this hurt someone else?

If it benefits you, it will probably benefit others. I'd encourage you to go for it. Submit a PR. Understand it may spend some time on the review queue, and might not make it in the next release.

tcaduser commented 10 months ago

I was surprised to learn that insertion order is not guaranteed on Python sets, while they are guaranteed on Python dicts.

I think consistent build order is important, so I support this effort.

I think using a list versus set would be a good idea. In other places, it may be good to use a dict, to preserve the insertion order.

Julian-O commented 10 months ago

I was surprised to learn that insertion order is not guaranteed on Python sets, while they are guaranteed on Python dicts.

Insertion order is only guaranteed on collections.OrderedDict, not all dicts.

OrderedDict can be used to simulate OrderedSet, by ignoring the values and using only the keys.

Julian-O commented 10 months ago

No, I am wrong! dicts also preserve insertion order, since Python 3.7! Sorry; I am working on out of date info.

UriyaHarpeness commented 10 months ago

Thanks for the feedback! I really appreciate it. I will start working on a PR soon I hope, and hoping it it won't take too long, as I'll try to spot maybe more such cases along the say.

As for the dict suggestion: they do preserve order, but set is more appropriate for the use of the container - only one of each item, while a dict serves more purpose than that alone. Also, I'd say that not only keeping the order, but having it by ABC, is very helpful when searching for something specific.

UriyaHarpeness commented 10 months ago
  1. Opened a PR: https://github.com/kivy/python-for-android/pull/2934
  2. Why aren't tools like black or isort used?
  3. Why isn't there any type annotation?
  4. Bonus: may I please get a support perhaps in the kivy-users group? https://groups.google.com/g/kivy-users/c/zQNiMEEOI4I
Julian-O commented 10 months ago

1: Yay 2: Inertia. It will take some effort to introduce them. (I want them; considering adding to a small project first.) 3: Inertia. The project pre-dates many of the recent developments. Feel free to start type annotations to your new code. 4: Sorry, can't help. Consider using the Discord.

UriyaHarpeness commented 10 months ago
  1. I can try to add them to this project, I assume it would be a bit harder to merge, but rebasing over and over would also be bad...
  2. Once the other PR is merged, I could perhaps try to add typings wherever I see in the main pythonforandroid folder, would such a PR that introduces a lot of changes which are only typing related be accepted?
  3. Sent a message there, I really hope to get the help there, since it's a failure that started some time ago without any change, on a clean environment.
Julian-O commented 10 months ago

2: No, please don't add black and isort ad hoc to a big project. You will cause merge and Git Blame issues, and make the people who haven't realised yet that they are the One True Way reject them.

Let's add it to a small alpha-stage project like kivy/ncis first, and show the core team that it is a good thing, before introducing to the major projects. There are techniques with GitHub to overcome the Git Blame issue (which seem like black magic to me, so I would rather see them tested).

In the meantime, every time I touch an import list, I have sorted it, and in a major refactor of a file I did recently, I ran it through black, because the git blame was going to be useless anyway.

3: I assume so, but probably worth advertising that you are going to do it on GitHub Discussions. The biggest bang-for-buck for type-hints are at major APIs rather than internal code. Kivy Widgets might be an ideal place to start.