Skip to content

Conversation

@QuietMisdreavus
Copy link
Contributor

@QuietMisdreavus QuietMisdreavus commented Sep 23, 2022

Bug/issue #, if applicable: rdar://77749746

Summary

This PR takes the table alignment info from Swift-Markdown and emits it into Render JSON, so that Markdown tables' column alignment information can be reflected in the final rendering.

Dependencies

swiftlang/swift-docc-render#441

Testing

Use the following table as test information:

| Column One | Column Two | Column Three | Column Four |
| :--------- | ---------: | :----------: | ----------- |
| one        | two        | three        | four        |

Steps:

  1. Add the above Markdown to Sources/SwiftDocC/SwiftDocC.docc/SwiftDocC.md.
  2. Clone and build the Swift-DocC-Render PR.
  3. DOCC_HTML_DIR=/path/to/swift-docc-render/dist bin/preview-docs
  4. Ensure that the table's columns have the proper alignment when rendered.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • [ n/a ] Updated documentation if necessary

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

franklinsch
franklinsch previously approved these changes Sep 26, 2022
self.rows = rows
self.extendedData = extendedData
self.metadata = metadata
if let alignments = alignments, !alignments.allSatisfy({ $0 == .unset }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems odd to not assign to the property if they're all unset here. Is there a technical reason that's needed? As a user of the API, I'd expect my argument to get assigned to the property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mainly for backwards compatibility with existing DocC-Render - the absence of the alignments list is the same as when they're all unset, so i decided to drop it to save on encoding space.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not emitting it in the encoder seems to be handled in the encode function though, right? It doesn't seem like the extra check here is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this check, the assertRoundTripCoding would fail, because [.unset, .unset...] is not equal to nil. I could change the Equatable conformance to allow these things to compare equal, but i decided to have the check here as well to use the default conformance instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, then it seems like we should implement this filtering in the decoder, rather than the initializer, that way clients that use the Swift model directly aren't affected by this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some filtering to the decoder and left the initializer alone, to allow all the means of creating the Table struct to get the same results. I've also added some comments describing the behavior of the initializer.

@franklinsch franklinsch dismissed their stale review September 26, 2022 09:33

Didn't meant to hit the approval button

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

1 similar comment
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

Comment on lines +241 to +249
/// A `nil` value for this property is the same as all the columns being
/// ``RenderBlockContent/ColumnAlignment/unset``.
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be inferred as this type's Equatable conformance implementing this behavior. Maybe we can mention that this is the behavior in practice or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we can actually override == and implement the behavior there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should a Table (with 3 rows and alignments are [.unset, .unset, .unset]) equal to a Table (with the same 3 rows but alignments are nil)?

Their actual behavior is the same but the current default Equatable implementation is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed up a new commit that implements this behavior into the Equatable conformance, and adds some tests to ensure that the behavior holds.

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus QuietMisdreavus merged commit ca2bc77 into swiftlang:main Oct 4, 2022
@QuietMisdreavus QuietMisdreavus deleted the table-align branch October 4, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants