kiwix / kiwix-apple-custom

GNU General Public License v3.0
2 stars 2 forks source link

Dynamically generate valid xcode project files from json #9

Closed BPerlakiH closed 10 months ago

BPerlakiH commented 11 months ago

Fixes #1 Fixes https://github.com/kiwix/apple/issues/603 Fixes https://github.com/kiwix/apple/issues/559 Fixes #10 Fixes https://github.com/kiwix/apple/issues/615

How it works

Screenshot 2023-12-31 at 15 06 12

next steps

other changes

kelson42 commented 10 months ago

@BPerlakiH Please explain what and how this repository works in the Readme. You can take example on the Android one.

BPerlakiH commented 10 months ago

@kelson42 I've added the README file.

BPerlakiH commented 10 months ago

I have a new path. I don't understand why we need and the purpose of the dwds/Categories directory?

Probably a left over from copying the whole assets folder. I think we should be good without that. I removed that now.

BPerlakiH commented 10 months ago

Thank you!

I think we lack explanations regarding how this technically works: I might have missed something but it looks like any push to main or PR will trigger a build and testflight beta for all custom apps. Is that what we want?

There doesn't seem to be a way to (even manually) trigger a single app build as well. It even looks like it's done step by step for all apps so a broken app would fail the whole thing. Having a single-app tool would allow to run it in parallel in github actions and even have a per-app reporting in the workflow.

On the python code, I have some general comments

  • generate_and_download.py: I don't see the value of this file at the moment. If it's entirely dependent on custom_apps.py and is entirely static code, maybe it should be move in __main__ of custom_apps.py
  • you should use pathlib module for filepath handling and not rely on strings. This will be handy for stuff like rglob
  • There's no dosctring and naming is not helping: CustomApps().append_to(): no clue from naming what this is supposed to do.
  • the chaining of string replacements eventually ending up being executed is terrifying 😅 This is extremely opaque and hard to maintain.
  • use f-strings instead of .format() ; it's clearer and in line with our convention.
  • split the Info holder and the command builder.
  • dosctring or comments for all commands
  • fix codefactor issues
  • use named groups for your regexp
  • there's a TODO:remove me comment. is it still necessary?
  • don't us the shell to perform simple operations for which there's an API (file copy for instance)
  • use subprocess (with arrays) when running processes

I have fixed these issues, please review. Next step is to look into the CI / CD separation, as mentioned above.

kelson42 commented 10 months ago

@BPerlakiH CI still fails!

BPerlakiH commented 10 months ago

The required changes were merged in other PRs.