-
Notifications
You must be signed in to change notification settings - Fork 163
Filter topics languages in articles #103
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,15 +39,19 @@ extension KeyedEncodingContainer { | |
| ) | ||
| } | ||
|
|
||
| /// Encodes the given variant collection. | ||
| /// Encodes the given variant collection for its non-empty values. | ||
| mutating func encodeVariantCollectionIfNotEmpty<Value>( | ||
| _ variantCollection: VariantCollection<Value>, | ||
| forKey key: Key, | ||
| encoder: Encoder | ||
| ) throws where Value: Collection { | ||
| try encodeIfNotEmpty(variantCollection.defaultValue, forKey: key) | ||
|
|
||
| variantCollection.addVariantsToEncoder( | ||
| variantCollection.mapValues { value in | ||
| // Encode `nil` if the value is empty, so that when the patch is applied, it effectively | ||
| // removes the default value. | ||
| value.isEmpty ? nil : value | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice 👍🏻 Will this apply to every time we encode a collection to a variant? (Are there other code paths that achieve this similar thing?) I ran into bugs where I would end up with an empty topic section instead of a removed one a few times so making this happen at the encoding level makes a ton of sense to me.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will apply every time we call
Yes, this is exactly what this code is fixing :) |
||
| }.addVariantsToEncoder( | ||
| encoder, | ||
|
|
||
| // Add the key to the encoder's coding path, since the coding path refers to the value's parent. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* | ||
| This source file is part of the Swift.org open source project | ||
|
|
||
| Copyright (c) 2022 Apple Inc. and the Swift project authors | ||
| Licensed under Apache License v2.0 with Runtime Library Exception | ||
|
|
||
| See https://swift.org/LICENSE.txt for license information | ||
| See https://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
| */ | ||
|
|
||
| import Foundation | ||
|
|
||
| public extension VariantCollection { | ||
| /// A variant for a render node value. | ||
| struct Variant<Value: Codable> { | ||
| /// The traits associated with the override. | ||
| public var traits: [RenderNode.Variant.Trait] | ||
|
|
||
| /// The patch to apply as part of the override. | ||
| public var patch: [VariantPatchOperation<Value>] | ||
|
|
||
| /// Creates an override value for the given traits. | ||
| /// | ||
| /// - Parameters: | ||
| /// - traits: The traits associated with this override value. | ||
| /// - patch: The patch to apply as part of the override. | ||
| public init(traits: [RenderNode.Variant.Trait], patch: [VariantPatchOperation<Value>]) { | ||
| self.traits = traits | ||
| self.patch = patch | ||
| } | ||
|
|
||
| /// Returns a new variant collection containing the traits of this variant collection with the values transformed by the given closure. | ||
| public func mapPatch<TransformedValue>( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this might be easier to understand at the call site without much added verbosity with just
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, the reason I called it
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I more meant- do we need this function at all? Couldn't I just do: <my-variant-collection>.patch.map { ... }Instead of <my-variant-collection>.patchMap { ... }?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, gotcha. So the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohhh. Tricky. Yup- that makes sense. Thanks! |
||
| _ transform: (Value) -> TransformedValue | ||
| ) -> VariantCollection<TransformedValue>.Variant<TransformedValue> { | ||
| VariantCollection<TransformedValue>.Variant<TransformedValue>( | ||
| traits: traits, | ||
| patch: patch.map { patchOperation in patchOperation.map(transform) } | ||
| ) | ||
| } | ||
| } | ||
| } | ||
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'm seeing this comment that we shouldn't add eyebrow text for collection pages, but then I don't see the corresponding
elsebranch where we do add eyebrow text for articles that don't have task groups. Am I missing something here?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.
Ah okay. Now I'm seeing that below outside of the topic sections variants bit.
Maybe the comment just doesn't apply any more and we should remove it? The ordering of when we set the eyebrow text versus set the topic sections shouldn't matter.
Uh oh!
There was an error while loading. Please reload this page.
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.
Final thought- we should probably make API collection pages only available in Objective-C or Swift if it curates languages of one or the other language.
Otherwise would need to call it an article in one variant and a collection group in the other which doesn't make sense.
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.
Hmm, that's an interesting edge case. I'm not convinced that API collection pages should only be available in the language in which their topics are curated, because the API collection page could contain prose content that's interesting regardless of the language. It's easy to create an API collection accidentally (i.e., by just adding topics to an Article). Maybe we can move this conversation to the forums. Separate from that, this would be technically challenging because we'd need to parse the contents of the page before assigning its available languages.
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.
Yeah that's fair. Maybe the right default behavior is that we should turn it into an Article in one language and an API collection page in the other.
Then as a future enhancement we should add a metadata directive that limits a given page to one language or the other.
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 like that idea. A directive that forces the languages in which an article (and framework, maybe?) is available. Something to discuss in more detail in the forums.
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 filed https://bugs.swift.org/browse/SR-15998 to track this.