Skip to content
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

add tuple support #69

Merged
merged 14 commits into from
Mar 15, 2020
Merged

add tuple support #69

merged 14 commits into from
Mar 15, 2020

Conversation

sahara-ooga
Copy link
Collaborator

@sahara-ooga sahara-ooga commented Mar 11, 2020

Support tuple.

let pretty = Pretty(formatter: SinglelineFormatter())
let tuple = (1, ("one", URL(string: "https://www.example.com/")!))
print(pretty.string(tuple, debug: false))    
//(1, ("one", https://www.example.com/))

/* label */
let labeledTuple = (2019, region: "Chili", variety: Optional("Chardonnay"), taste: ["round", "smooth", "young"])
print(pretty.string(labeledTuple, debug: false))
//(2019, region: "Chili", variety: "Chardonnay", taste: ["round", "smooth", "young"])

this solves #34.

@sahara-ooga sahara-ooga marked this pull request as ready for review March 12, 2020 09:56
# Conflicts:
#	Sources/Core/Formatter/MultilineFormatter.swift
Copy link
Owner

@YusukeHosonuma YusukeHosonuma left a comment

Choose a reason for hiding this comment

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

Good works!!

Sources/Core/Pretty.swift Outdated Show resolved Hide resolved

func testTupleString() {
assert(to: formatter.tupleString).expect([
when([], then: #"()"#),
Copy link
Owner

Choose a reason for hiding this comment

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

Good 👍

Tests/Core/Formatter/SinglelineFormatterTests.swift Outdated Show resolved Hide resolved
Tests/Core/PrettyTests.swift Show resolved Hide resolved
Apply suggestions from code review.
@codecov-io
Copy link

codecov-io commented Mar 13, 2020

Codecov Report

Merging #69 into master will increase coverage by 2.69%.
The diff coverage is 94.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
+ Coverage   74.84%   77.53%   +2.69%     
==========================================
  Files           8        8              
  Lines         318      365      +47     
==========================================
+ Hits          238      283      +45     
- Misses         80       82       +2
Impacted Files Coverage Δ
Sources/Core/Pretty.swift 86.77% <100%> (+0.97%) ⬆️
Sources/Core/Formatter/SinglelineFormatter.swift 95.45% <90.9%> (-4.55%) ⬇️
Sources/Core/Formatter/MultilineFormatter.swift 98.03% <93.33%> (-1.97%) ⬇️
Sources/API/Debug.swift 71.11% <0%> (+6.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf83e00...8990689. Read the comment docs.

thanks to Yusuke, I get the image that we can show the merit to support tuple type. A tuple can hold multi type element, which is distinct from Array.
Copy link
Owner

@YusukeHosonuma YusukeHosonuma left a comment

Choose a reason for hiding this comment

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

Good works!!

Tests/Core/Formatter/MultilineFormatterTests.swift Outdated Show resolved Hide resolved
@@ -66,6 +66,35 @@ class MultilineFormatterTests: XCTestCase {
]
""")
}

func testTupleString() {
let tupleElements: [(String?, String)] = [
Copy link
Owner

Choose a reason for hiding this comment

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

What time first argument of tuple is nil?
(Please write to test that passed to nil if possible)

Copy link
Collaborator Author

@sahara-ooga sahara-ooga Mar 15, 2020

Choose a reason for hiding this comment

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

I can not find a situation in which the label is nil.

Example

the tuple is ()

a formatter receives [].

no label, one element

the tuple is transformed to the inner element instance.

(#""one""#) becomes "one".

no label element and labeled element

(#""one""#, second: 2) becomes [(".0", "\"\"one\"\""), ("second", "2")].

Tests/Core/PrettyTests.swift Outdated Show resolved Hide resolved
// if the labels of tuples are not specificated, they are like ".1" (not nil).
// Using "." as the first charactor of the label of tuple is prohibited.
let labelValuePairs: [String] = elements.map { label, value in
if let label = label, label.first != "." {
Copy link
Owner

Choose a reason for hiding this comment

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

I think a better way would be this judge move to Pretty.swift.

For example:

// Pretty.swift
case .tuple:
    let elements: [(String?, String)] = mirror.children.map {
        var labelOrNil: String? = nil
        if let label = $0.label, label.first != "." {
            labelOrNil = label
        }
        return (label: labelOrNil, value: _string($0.value))
    }
    return formatter.tupleString(elements: elements)

// Here
let labelValuePairs: [String] = elements.map { label, value in
    if let label = label {
...

Formatter should be concentrate styling therefore should not judge property of data.

Copy link
Collaborator Author

@sahara-ooga sahara-ooga Mar 15, 2020

Choose a reason for hiding this comment

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

Thank you for a comment!

This suggestion is so reasonable, I adopt it in b4676ce.

Sources/Core/Formatter/SinglelineFormatter.swift Outdated Show resolved Hide resolved
sahara-ooga and others added 5 commits March 15, 2020 10:50
followed @YusukeHosonuma's suggestion

Co-Authored-By: Yusuke Hosonuma <tobi462@gmail.com>
along with it, modify test cases of single line formatter because formatters receive proper labels.
Co-Authored-By: Yusuke Hosonuma <tobi462@gmail.com>
# Conflicts:
#	Sources/Core/Pretty.swift
#	Tests/Core/PrettyTests.swift
Copy link
Owner

@YusukeHosonuma YusukeHosonuma left a comment

Choose a reason for hiding this comment

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

Thank you for your polite works ❤️
Please check last suggestion comment from me :)

That's all!!

Sources/Core/Formatter/MultilineFormatter.swift Outdated Show resolved Hide resolved
sahara-ooga and others added 2 commits March 15, 2020 14:07
Co-Authored-By: Yusuke Hosonuma <tobi462@gmail.com>
@sahara-ooga sahara-ooga merged commit 92e8530 into master Mar 15, 2020
@sahara-ooga sahara-ooga deleted the feature/tuple branch March 15, 2020 05:34
@sahara-ooga sahara-ooga mentioned this pull request Mar 15, 2020
@sahara-ooga
Copy link
Collaborator Author

Tuple対応

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

Successfully merging this pull request may close these issues.

None yet

3 participants