Skip to content
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.

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

mkpack: add image resources #130

Merged
merged 3 commits into from Oct 23, 2018
Merged

mkpack: add image resources #130

merged 3 commits into from Oct 23, 2018

Conversation

Helco
Copy link
Collaborator

@Helco Helco commented Oct 21, 2018

these are crushed with a function imported from the SDK.

Requirement for this is the json declaring whether the target display has colors or not, as such this should be discussed and then applied to the resource repo (see pebble-dev/RebbleOS-resources#5)

these are crushed with a function imported from the SDK
def expand_path(path):
if path.startswith('~'):
path = os.path.expanduser(path)
return path

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return os.path.expanduser(path)? the function returns args unchanged if there's no leading tilde :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately not with python 2.7 (which the pebble sdk is using), where it raises an exception if the path does not start with '~'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. Works in 2.7.15 for me. ¯_(ツ)_/¯

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed I cannot longer reproduce this behaviour, I must have misread the error message

@jwise
Copy link
Collaborator

jwise commented Oct 22, 2018

Looks good. I wonder if there is a more Pythonic way to have crush_png as a global (as if it were imported), rather than passing it around everywhere. Having a crush_png = None at the top of the file next to the imports, and then filling it in later, seems like my intuition, but I'm not sure if that's actually any better. @Katharine, do you have style advice here?

@jwise
Copy link
Collaborator

jwise commented Oct 22, 2018

Holy shit that went bad after a lot of "you can't comment at this time". Thanks, GitHub.

@jwise
Copy link
Collaborator

jwise commented Oct 23, 2018

Ok, since this is blocking #131 , let's just merge it, unless you can think of a cleaner way to deal with that. LGTM to merge! Also consider this 'pre-approval' for a PR to update the submodule ref for resources; I guess this has to go in first, then resources update, then a submodule ref here for the resources update, then #131.

@Katharine
Copy link

Katharine commented Oct 23, 2018

Aha, here this PR is. I saw this in my email and then lost it.

I agree with the approach of having crush_png = None as a global, followed by attempting to import it - this is a fairly idomatic approach to imports that can tolerate failure. I would, however, probably attempt the import early on, so as to avoid surprises where it's not imported even though it could've been.

@Helco
Copy link
Collaborator Author

Helco commented Oct 23, 2018

Why not resources first, then immediatetly this with the updated submodule ref and then #131?
Breaking the repo for a minute shouldn't be to bad yet

(btw: it seems like my right to merge has expired)

@jwise jwise merged commit 8e7ae6a into ginge:master Oct 23, 2018
@jwise
Copy link
Collaborator

jwise commented Oct 23, 2018

That's fine too. Anyway, I've merged this, and will merge the resources change in a minute...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants