-
Notifications
You must be signed in to change notification settings - Fork 63
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
Make SFSymbol struct #72
Conversation
@Stevenmagdy Thanks for starting this! If you need any help with the CI, let me know! |
@fredpi yeah, it fails but no logs are shown. |
@fredpi All tests have passed. |
@Stevenmagdy Thanks again! Just so that you know: I have quite a busy week ahead and will probably only find time to review this in a week or so :) |
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.
Just a quick look. I'll do full review later.
Maybe it doesn't do harm if we explicitly state conformance even if the |
I’d also like to have |
@Stevenmagdy Thanks again for your great work and sorry for my much delayed review! Except for the 3 comments I left, everything looks very good to me. |
I think Adding retroactive conformance for a type you don't own to a protocol you don't own is considered bad practice in Swift. So if we don't add |
I'm indifferent on this, but I like to follow Apple not conforming to |
@ddddxxx The issue I see with Of course this ambiguity also causes issues with Actually I'm quite indifferent on this topic, but just don't want to introduce ambiguous behavior (if that's somehow possible without destroying multiple valid use cases while at it). |
@Stevenmagdy I've given you write access, so if you want, you may push your branch to the original repo (instead of your fork) and continue work there |
I think this ambiguity will always be there as long as, we are conforming to My issue with So in case, we don't want to introduce support for custom symbols, I think we should conform to |
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.
It seems that you submitted SymbolEnumCreator/.build/manifest.db
, ignore it?
Are you sure? I think it was actually tracked and I ignored it in this PR. |
RawRepresentable
I still don't understand why there is any use case for things like @ddddxxx wrote the following:
But are any of these special treatments something one may want to use if one knows about the ambiguity of the Custom Symbol Initializer
This is a very important observation, I didn't think about that. So the only use case for a @Stevenmagdy i just want to make sure we make a good decision regarding the |
I think v3 might have solved this issue for us. @fredpi This ambiguity was introduced due to rawValue being determined based on the platform version, mostly because of the renamed symbols.
This reason was true in The user now will know that I think conveniences for accessing the most recent version of a symbol (i.e. Why
|
I'm just gonna throw in my opinion here: HashableIt makes much sense for
RawRepresentableThe question remains with Explicitly supporting But I would like to see a concrete example of why we should support |
@knothed You are right, There isn't a use case that I can think of. But I think that's also the case with any type conforming to
I actually agree that we shouldn't conform to |
Hey @fredpi, What is the status of this PR? I hope it will be merged before the release of iOS 15 🙏 |
@Stevenmagdy I'm super sorry for the large delay! Life was getting in the way, but that shouldn't count as an excuse for such a long period of inactivity. Looking forward, I'm now committed to merging this and releasing v3 support ASAP, i.e. in the next week. Looking back at the discussion in this issue, to my understanding it is best to remove
Or am I missing something again? @knothed @Stevenmagdy If we can't find a quick solution without discussing this for the next three months, I'd suggest we just merge this. But before, let's see if we can make a quick, informed decision. |
Yes, my argument is that we should not make it unstable across executions of the program, and make This will allow us to conform to The changes discussed are already implemented in this PR. @fredpi @knothed I apologize for my poor English, I hope I made my point clear. |
@Stevenmagdy makes sense, but what if Apple decides to rename some symbols in, say, SFSymbols 4? |
We are already supporting that case in the |
Okay so I‘m fine with either way, conforming to RawRepresentable or not. |
@fredpi Should we go ahead and merge this? I will follow it with another PR for the v3 symbols. |
@Stevenmagdy Thanks for your explanation, now I get it! I will now merge this. 😃 Thank you again for your great contribution, I really appreciate it!! If you would also follow up with a v3 symbol PR, that'd be great! By the way: I don't really understand why you think that your English is poor – it's perfectly understandable and probably better than mine! |
Fixes #65