Clean up to make building MacOS Package work... #77
Conversation
… as .pdf does not exist.
…and setup. MacOS build instructions.
|
This would fix issue #75 |
setup.py
Outdated
|
|
||
| setuptools.setup( | ||
| name="open-mcr", | ||
| url = "https://github.com/gutow/open-mcr", |
There was a problem hiding this comment.
We should make this the URL of the original repo:
| url = "https://github.com/gutow/open-mcr", | |
| url = "https://github.com/iansan5653/open-mcr", |
setup.py
Outdated
| long_description=long_description, | ||
| long_description_content_type="text/markdown", | ||
| author="Ian Sanders", | ||
| author_email="[email protected]", |
There was a problem hiding this comment.
I'd just prefer to use my personal email for this (I might not work at GitHub forever 😄):
| author_email="iansan5653@github.com", | |
| author_email="iansan5653@gmail.com", |
src/user_interface.py
Outdated
| helpfile = str(Path(__file__).parent / "assets" / "manual.pdf") | ||
| subprocess.Popen([helpfile], shell=True) | ||
| helpfile = helpfile = os.path.join(Path(__file__).parent, "assets", | ||
| "manual.pdf") |
There was a problem hiding this comment.
Are you sure os.path.join is necessary here? Notice that in the original code we are not combining strings but instead using the slash (/) operator, which should join paths similarly to os.path.join. I think that the / way is more readable if it does work on all systems.
For reference, here's the documentation on the slash operator.
There was a problem hiding this comment.
I have not investigated the newer path module. However, I am guessing it rides on top of os.path. In interpreted code I generally try to avoid abstractions as much as possible. I know that os.path.join works across all platforms. It does look like your usage of path works across all platforms as you use it other places as well, but I am not confident of how robust it is. I also am wary of overloading fundamental operators such as divide /.
There was a problem hiding this comment.
Let's keep with the current style please, since it's more readable and consistent across all the code. The new path module should work in all situations, and I think it's the more modern/actively updated library.
There was a problem hiding this comment.
Hi, just looking at this PR again as I'd love to get it merged and get a new release out soon. I like these changes and I'm looking forward to being able to distribute a MacOS package!
I think if you can complete the following then I can merge this. It's mostly just trimming off some of the extra changes to focus this PR on just the MacOS fixes:
- Revert the scrolling frame changes
- Revert the change from using
/to join paths (if you do find this causes any issues, please open an issue and I can take another look at it) - Remove the
test_files -- I definitely agree with having more tests but we can open issues instead of adding TODO comments, that way all the work is tracked in one place - Revert the requirements change from
typing==3.7.4.1toPillow==9.2.0; this looks like a mistake (please correct me if I'm wrong!)
I did notice that the fixed version of the PDF has the fonts missing (everything is in Times New Roman). I don't think this is a major issue though and I'd still rather merge this version since it's definitely better than a broken one.
| PyInstaller==3.5 | ||
| typing==3.7.4.1 | ||
| PyInstaller>=4.6 | ||
| Pillow==9.2.0 |
There was a problem hiding this comment.
I like the change to PyInstaller>=4.6, but I'm confused by the change from typing (the type-checking tool) to Pillow (an image processing library). Was that an intentional change?
There was a problem hiding this comment.
Pillow is the python library to use the PIL image library package. I ended up with it for something, maybe PyInstaller. I will look back at it. typing is not required for python 3.9+ as it is included. As the current python is 3.10, I am trying to update this to work with that.
There was a problem hiding this comment.
I think I will get to the rest of these this weekend.
There was a problem hiding this comment.
typingis not required for python 3.9+ as it is included
Oh I didn't know that. 👍 to removing the dependency then, thanks!
There was a problem hiding this comment.
Still have to figure out why I had to add Pillow. I think it is for icon building for MacOS.
-
Revert the scrolling frame changes
-
Revert the change from using / to join paths (if you do find this causes any issues, please open an issue and I can take another look at it)
-
Remove the test_ files -- I definitely agree with having more tests but we can open issues instead of adding TODO comments, that way all the work is tracked in one place
-
Revert the requirements change from typing==3.7.4.1 to Pillow==9.2.0; this looks like a mistake (please correct me if I'm wrong!)
Most important changes are updates to allow building MacOS app:
os.path.join().Less important: