-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
Bump ggez to version 0.5.0-rc.2 #485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for the investigation!
Though, other changes are controversial (see review comments). Could you remove them from the PR?
.gitignore
Outdated
@@ -1,3 +1,4 @@ | |||
/target | |||
/assets | |||
/static | |||
.DS_Store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that a project's gitignore file should only list project-specific things and all globally ignored files should be placed in a global gitignore file - https://stackoverflow.com/questions/18393498/gitignore-all-the-ds-store-files-in-every-folder-and-subfolder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding it would serve more good than harm. I've always added .DS_Store
to project repos to help make developability more seamless given there are so many developers on macOS, as most user's haven't had time to configure their global gitignore (and some aren't even aware of it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ozkriff I understand your argument, but imo there is different purpose of both of those files:
- local global
.gitignore
prevents me from committing.DS_Store
- project-specific
.DS_Store
prevents anyone from accidentally committing.DS_Store
I believe the later solution is a desired one. Otherwise a reviewer of the pull request has to always check whether .DS_Store
was not accidentally committed.
btw. that's rustc's .gitignore
https://github.com/rust-lang/rust/blob/d85e866c0d28fce32856d200fd534ac1c2c721c8/.gitignore#L3
.editorconfig
Outdated
[*] | ||
indent_style=space | ||
indent_size=tab | ||
tab_width=4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe that editorconfig is worth having in a repo when the code is always formatted with cargo-fmt (and this is checked on travis-ci)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having .editorconfig may help users that use Visual Studio code, since new lines of code are formatted according to that config. See https://docs.microsoft.com/en-us/visualstudio/ide/create-portable-custom-editor-options?view=vs-2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ozkriff both, my editor's (vim) and my cargo-fmt default settings are different to the one used in this repo. This file really makes life easier if you're switching between multiple projects and you don't remember/don't care what kind of formatting they use
@ozkriff thanks for the review. I understand your concerns. I removed unrelated changes from this pr and opened a new pr only with |
Thanks, merged. :) I'll think over your DS_Store and editorconfig proposals. |
Issue #408 and #484 are both caused by the bug in ggez's screen resizing on mac os. Version
0.5.0-rc.2
of ggez disabled support for hdpi screens, so there is no automatic resize at app launch. Thanks to that mouse click offset issue reported in #408 no longer exists.closes #408