Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Fonts: fix code which checks if a font is already registered #77

Merged
merged 4 commits into from
Aug 22, 2017

Conversation

chipp
Copy link
Member

@chipp chipp commented Aug 22, 2017

@AliSoftware we discussed these changes here 06bcd2a#diff-3fc62affc2b681781b6ef4a2fac1a6e7R37

Also I prepared test project for these changes: https://github.com/chipp/FontTests

I tried to reuse NSFontManager.shared().availableMembers(ofFontFamily:) solution for macOS, but with help of test project I found that NSFontManager doesn't see freshly added fonts and this check is useless.

@djbe
Copy link
Member

djbe commented Aug 22, 2017

I remember trying the registration on macOS when I first wrote it. It seemed to work at the time 😅

Seeing as CI compilation passes, the only thing missing is a changelog entry. If you could please add it crediting yourself, and using the same format as the other entries. See here for more information:
https://github.com/SwiftGen/SwiftGen/blob/master/CONTRIBUTING.md#requirements--conventions

CHANGELOG.md Outdated

### Bug Fixes

* Fix code which checks is font already registered.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: "if font"

Also, please add two spaces after that ending period. That's required by MarkDown syntax to render the next line separately properly (see other entries for inspiration)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see @djbe beat me to it and already made the change for you ^^


fileprivate var url: NSURL? {
let bundle = NSBundle(forClass: BundleToken.self)
guard let url = bundle.URLForResource(path, withExtension: nil) else { return nil }
Copy link
Collaborator

@AliSoftware AliSoftware Aug 22, 2017

Choose a reason for hiding this comment

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

Wait, given than you guard let url … else return nil and do nothing afterwards but directly return url, why not directly return bundle.URLForResource(path, withExtension: nil) there? That would already directly return nil if URLForResource return nil, so…

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, you right..

Copy link
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

LGTM now 👌
Thanks for the contribution @chipp 👍

@chipp chipp changed the title Fix code which checks is font already registered Fonts: fix code which checks if a font is already registered Aug 22, 2017
@AliSoftware AliSoftware merged commit 4f09ab8 into SwiftGen:master Aug 22, 2017
@AliSoftware
Copy link
Collaborator

Thanks a lot for contributing to SwiftGen @chipp ! 👍
To thank you for your work and for taking time to improve the tool, I've invited you to join SwiftGen's GitHub organization's "CoreContributors" team 😃

No pressure to accept nor any obligation in that! It will just give you the ability to help triage issues or push directly branches to the repo (instead of your fork)… in case you want to contribute again in the future 😉

For more information, read the COMMUNITY.md and CONTRIBUTING.md documents, and if you have any questions don't hesitate to reach out!

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

Successfully merging this pull request may close these issues.

3 participants