-
-
Notifications
You must be signed in to change notification settings - Fork 87
Swift 4.2 and quality of life improvements #82
Conversation
* Update .swiftlint.yml to allow enum names
… into joeljfischer-swift-4-update
…s can be easily generated
Wow this is pretty awesome! Thanks for doing this @robbiet480. |
@robbiet480 I pushed one PR fix for swift 4.2 compile error. Could you check it out. |
Fix swift 4.2 compile error
@dzenbot Just checking in on the review of this! For those using this work, just a heads up that I have noticed significant slowdowns in my archive times in Xcode 10.2 and Swift 5, mostly due to Iconic. According to this comment I found when researching the problem, it's due to our very large Here's what my build looks like, notice "Compile Swift Files" is taking the longest amount of time, almost 30 minutes: |
@dzenbot Also, not sure if the delay in review is perhaps due to you no longer using this library. If so, I am more than happy to takeover long term development of it and transfer it to my GitHub. Let me know! |
Hey @robbiet480! Yes, I'm sorry, I no longer use this library for any projects I'm working on, so it's been in my lower-priority-bucket unfortunately. Do you want to be a contributor? You'd have privileges to push changes and everything, but the repository would still be under my account. |
@dzenbot Sure, but can you make me full admin in case I need to clean up spam and the like, as well as be able to manage versions and labels. |
@dzenbot Hi, I still haven't heard back from you in a while and this PR has still gone unreviewed and unmerged after sitting here for almost 9 (!) months now. I need to know what your short and long term plans are for Iconic. There's a lot of people that are continuing to depend on it, both visible in this issue and elsewhere, and it hasn't seen love for a long time now. I worry at this point about even becoming a admin let alone contributor because in case I needed you for something I don't think I could contact you in a timely manner. Here's what I'm proposing for now:
If you aren't comfortable with my plan above, then I'm open to alternatives, but whatever it is I just need to be comfortable in our shared understanding about timeliness of reviews and merges as well as some alternative communication method that I can contact you at. I don't ever expect you to jump on anything happen instantly, overnight, or even within a week really, but 9 month delays on PRs that will actually make the library work again are unfair to me and the users of Iconic. If it was a simple feature add, this would be a different thing, but as it stands Iconic just doesn't work for someone finding it today. If we can't come to a shared understanding then I plan to create a entirely new project and advertise it as a modern and supported replacement for Iconic so that users aren't left out in the cold any longer because my research hasn't found anything like Iconic available. I'd rather not do this, because Iconic is a great name, it has a great community and a good history which I'd rather get back to perfect from its less than perfect status right now vs having to build all of that for myself again. You have done great work on Iconic, it's a fantastic library which is the most powerful of any Swift icon font library available, even today, and you should be commended on it. I understand that situations change and you aren't using this library anymore, that's totally fine. But since it seems you won't be a good steward moving forward (since you aren't using it anymore) I am hoping to pick up where you left off. I have been using Iconic for almost 2 years now and have made it a cornerstone of Home Assistant Companion. There's a lot of work that I want to do to Iconic in the near term like upgrading to Swift 5 and getting a workaround in place for the Swift 5 build times, but I can't do that if PRs aren't getting merged, reviewed or even acknowledged for months at a time. I've just now submitted a PR to add support to SwiftGen for extracting code points from a font to be used in a template. This will allow Iconic to finally move away from the hacked version of SwiftGen contained in this repo that isn't even buildable anymore because it hasn't been updated since Swift 2.3 (and I've been unable to update it as such a old version of Swift seems impossible to run on a modern Mac). Finally, I'll apologize for coming off so strongly about all of this, but I am just supremely frustrated now with how long this pretty straightforward PR has taken to get merged or for me to even hear back from you. I'm sure that as a fellow developer, if the shoe was on the other foot, you would be just as frustrated as I am now. Thanks for reading @dzenbot and hopefully understanding my feelings on this. |
Hey @robbiet480, I understand your frustration, I've been there many times myself in the past. I totally forgot about this tbh and I also blame GH notifications being very silent. Anyhow, I'm willing to transfer the ownership of this repository to the home-assistant account. I feel less excited to transfer it to an individual account than to an actual community group. Mention on the README.md and AUTHORS.md and other relevant files as the original author of the library, sounds fair and more than enough to me. Finally, having admin access to this repo, even though I might not really use these rights, would make me happy. Let me know. I can flick the switch whenever you want. |
@dzenbot Transferring to @home-assistant is totally fine too, I am a admin and can accept the transfer any time. I have no problem with adding you as an admin either. |
@dzenbot Oh sorry, one other modification to the plan because we are transferring to @home-assistant: All repos under the organization are required to have a CODE_OF_CONDUCT.md file and a LICENSE.md file. I assume you are fine with both (you already have the LICENSE file, I'll just reformat it to be Markdown and add the .md file extension). We prefer to have everything licensed Apache 2.0 as per our governance requirements but can make an exception for this if you feel strongly about keeping MIT. License would be in effect as of the next release of Iconic. |
@dzenbot Finally, can you just let me know once you start the transfer? Thanks! |
@dzenbot Just tested transferring a new repo in with a user who doesn't have permissions. Seems I need to give you admin permissions to the organization temporarily to allow you to transfer in the repo. Doing so now. |
You should be able to accept the invite at https://github.com/home-assistant (or via the link in your email). Once you've done that I can increase your permissions. Alternatively to all of this, you should be able to add me as a admin to dzenbot/Iconic and then I can start the transfer process. |
@robbiet480 I've accepted the invite. Mind bumping the permissions temporarily? |
@dzenbot Yep, standby. I am now giving you permissions to transfer. You had to accept the invite first. :) |
@dzenbot You should be good to transfer now, let me know. |
@robbiet480 Ok, Iconic has a new home now! https://github.com/home-assistant/Iconic |
@dzenbot Thanks so much! Just wanted to confirm if you had a reason for me to keep the license as MIT or if I can migrate it to Apache 2.0. Also, don't be alarmed if you see some "removed from team" notifications from GitHub in the next few minutes, just organizing teams in general. Will ensure you are left with admin access. |
@dzenbot Another thing, can you please add me to the Pod so I can publish? Should just need to run |
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.
Looks great! Just a small nits. Maybe something to tackle separately.
} | ||
|
||
s.prepare_command = "sh Source/Iconizer.sh '#{ENV['FONT_PATH']}' --verbose" | ||
s.prepare_command = "sh Source/Iconizer.sh '#{ENV['FONT_PATH']}' '#{ENV['CUSTOM_FONT_NAME']}'" |
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'd suggest to keep the --verbose
to see what's going on the console at all times.
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 removed --verbose
because Iconizer.sh doesn't actually do anything with that flag.
@robbiet480 Apache 2.0 sounds good to me. Thanks. |
Hi @jkyin, @stevenp, @zvving, @joeljfischer, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
Everyone that just got notified, don't worry about the CLA, it's not gonna apply on this PR. |
This adds support for Swift 4.2. It also allows for setting a custom name for the font via the Iconizer.sh script.