python-for-android icon indicating copy to clipboard operation
python-for-android copied to clipboard

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

Open UriyaHarpeness opened this issue 2 years ago • 9 comments

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

UriyaHarpeness avatar Dec 04 '23 09:12 UriyaHarpeness

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

How might this hurt someone else?

  • Amount of additional processing, sorting a lists of 3 elements: Trivial
  • Increased chance of a bug: Trivial
  • Review time of a core developer: Small

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.

Julian-O avatar Dec 04 '23 13:12 Julian-O

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.

tcaduser avatar Dec 04 '23 14:12 tcaduser

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 avatar Dec 04 '23 15:12 Julian-O

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

Julian-O avatar Dec 04 '23 15:12 Julian-O

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 avatar Dec 04 '23 19:12 UriyaHarpeness

  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

UriyaHarpeness avatar Dec 05 '23 10:12 UriyaHarpeness

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.

Julian-O avatar Dec 05 '23 11:12 Julian-O

  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.

UriyaHarpeness avatar Dec 05 '23 12:12 UriyaHarpeness

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.

Julian-O avatar Dec 05 '23 13:12 Julian-O