Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make emoji and checkstyle reporter to output report sorted by file name #1430

Merged
merged 3 commits into from
Apr 14, 2017

Conversation

norio-nomura
Copy link
Collaborator

Fixes #1429

@SwiftLintBot
Copy link

SwiftLintBot commented Apr 12, 2017

12 Messages
📖 Linting WordPress with this PR took 12.3s vs 12.44s on master (1% faster)
📖 Linting Alamofire with this PR took 3.15s vs 3.14s on master (0% slower)
📖 Linting Firefox with this PR took 17.87s vs 17.25s on master (3% slower)
📖 Linting Kickstarter with this PR took 17.61s vs 17.87s on master (1% faster)
📖 Linting Moya with this PR took 0.42s vs 0.42s on master (0% slower)
📖 Linting Nimble with this PR took 1.65s vs 1.74s on master (5% faster)
📖 Linting Aerial with this PR took 0.43s vs 0.42s on master (2% slower)
📖 Linting Realm with this PR took 2.69s vs 2.75s on master (2% faster)
📖 Linting SourceKitten with this PR took 1.15s vs 1.23s on master (6% faster)
📖 Linting Sourcery with this PR took 2.86s vs 2.9s on master (1% faster)
📖 Linting Swift with this PR took 10.79s vs 10.88s on master (0% faster)
📖 Linting Quick with this PR took 0.52s vs 0.53s on master (1% faster)

Generated by 🚫 danger

@norio-nomura
Copy link
Collaborator Author

String comparison result is different between mac and Linux ... 😞

$ TOOLCHAINS=org.swift.31120170327a swift
swift_oss_helper command enabled.
warning: (x86_64) /Library/Developer/Toolchains/swift-3.1-RELEASE.xctoolchain/System/Library/PrivateFrameworks/LLDB.framework/Resources/repl_swift empty dSYM file detected, dSYM was created with an executable with no debug info.
Welcome to Apple Swift version 3.1 (swift-3.1-RELEASE). Type :help for assistance.
  1> "Other" < "filename"
$R0: Bool = true
  2>  
$ docker run --privileged -it -v `pwd`:`pwd`:cached -w `pwd` --rm norionomura/sourcekit:31 swift
Welcome to Swift version 3.1 (swift-3.1-RELEASE). Type :help for assistance.
  1> "Other" < "filename"
$R0: Bool = false
  2>  

@norio-nomura
Copy link
Collaborator Author

SR-530

@norio-nomura
Copy link
Collaborator Author

StringComparable.swift#L111-L112:

  public  // @testable
  func _compareString(_ rhs: String) -> Int {
#if _runtime(_ObjC)
    // We only want to perform this optimization on objc runtimes. Elsewhere,
    // we will make it follow the unicode collation algorithm even for ASCII.
    if _core.isASCII && rhs._core.isASCII {
      return _compareASCII(rhs)
    }
#endif
    return _compareDeterministicUnicodeCollation(rhs)
  }

When comparing ASCII character strings, there seems to be a difference depending on whether ObjC runtime is present:

  • If present, the same behavior as Foundation
  • If not, the behavior defined by Unicode

I think that the value expected for our test results should be switched depending on whether ObjC runtime is present or not.

@norio-nomura norio-nomura changed the title Make emoji and checkstyle reporter to output report sorted by file name [WIP] Make emoji and checkstyle reporter to output report sorted by file name Apr 13, 2017
@norio-nomura norio-nomura changed the title [WIP] Make emoji and checkstyle reporter to output report sorted by file name Make emoji and checkstyle reporter to output report sorted by file name Apr 14, 2017
@norio-nomura
Copy link
Collaborator Author

Updated.

@codecov-io
Copy link

codecov-io commented Apr 14, 2017

Codecov Report

Merging #1430 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1430      +/-   ##
==========================================
+ Coverage   81.13%   81.14%   +0.01%     
==========================================
  Files         180      180              
  Lines        9184     9190       +6     
==========================================
+ Hits         7451     7457       +6     
  Misses       1733     1733
Impacted Files Coverage Δ
Tests/SwiftLintFrameworkTests/ReporterTests.swift 84.44% <100%> (+0.72%) ⬆️
...e/SwiftLintFramework/Reporters/EmojiReporter.swift 90.47% <100%> (+0.47%) ⬆️
...ftLintFramework/Reporters/CheckstyleReporter.swift 93.54% <100%> (+0.21%) ⬆️

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 f36443f...45049e8. Read the comment docs.

Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

🎉

@jpsim
Copy link
Collaborator

jpsim commented Apr 14, 2017

Also, way to fix this in Swift! 👏

(swiftlang/swift#8769)

@jpsim jpsim merged commit 3fc7620 into master Apr 14, 2017
@jpsim jpsim deleted the nn-sort-report-by-filename branch April 14, 2017 18:51
@norio-nomura
Copy link
Collaborator Author

Thanks! 🙏

Also, way to fix this in Swift! 👏

Apparently, swiftlang/swift#8769 seems to be incompatible with the String Manifesto.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants