SourceKitten icon indicating copy to clipboard operation
SourceKitten copied to clipboard

[WIP] Refactor `Request` to make it resilience.

Open ainopara opened this issue 7 years ago • 9 comments

Motivation

We can find out some undocumented parameters / requests from SourceKit source code. Since they are not documented, those parameters of request may change over time. It will be great if we can support those parameters and evolve our requests to adjust SourceKit protocol changes over time without break current code. But we can not reach this target in current enum Request model.

Enum case can not overload and do not support default value for its associated values. That means if we add a parameter to a enum item, all projects rely on SourceKittenFramework should add the parameter in their code. That will also be verbose if we have to explicitly set all parameters even when we do not need to set it.

What does this PR do?

  • Make Request a struct. It can also be a class, but not enum. Because enum associated value can not have default value. If we want to add a new parameter for a request, we have to break the API. By making Request a struct (or class), we can add static function which has the same name and parameters of previous enum item to keep the expression Request.editorOpen(file: file).send() works without caller change their code. After this PR, we can add / remove parameters for a request while keep our API unchanged. We need to introduce new static function to expose new parameters and mark old static function deprecated.
  • As a result of adding new parameters some new api exposed to public. For example:
public static func editorOpen(
        name: String,
        source: EditorOpen.Source,
        enableSyntaxMap: Bool? = nil,
        enableStructure: Bool? = nil,
        enableDiagnostics: Bool? = nil,
        syntacticOnly: Bool? = nil,
        arguments: [String] = defaultArguments
        ) -> Request
  • Add SourceKitDef.swift which is generated by Sourcery with data from ProtocolUIDs.def at $(swift-root)/tools/SourceKit/include/SourceKit/Core/ProtocolUIDs.def.
  • Introduce RequestBuilder to help SourceKittenFramework users to build custom SourceKit request.
  • Impliment EditorOpenInterface, ModuleGroups and ProtocolVersion three kinds of request.
  • ... and add tests for them.
  • At last: Move overwrite to parameter of function compareJSONString to make update fixture easier.
  • Print which version of fixture we are loading when testing.

ainopara avatar Oct 06 '17 15:10 ainopara

Thanks for the PR! I must admit, I'm a bit scared by the sheer scope of these changes though and may not find the courage to review 😅

image

jpsim avatar Oct 23 '17 17:10 jpsim

image The most majority of these changes is from SourceKitDef.swift and SwiftAssertInterface.json.

The former is a file generated from ProtocolUIDs.def in apple/swift repo (clang preprocessor and Sourcery is used in this process). You can only review the structure of this file.

The latter is just a fixture used by test case testEditorOpenInterface which is safe to be ignored in review.

Things need to be reviewd are changes in Request.swift and the RequestBuilder.swift file which is less than 800 lines of change in total.

Recommended review route is begin with SourceKitDef.swift then RequestBuilder.swift then Request.swift then others.

ainopara avatar Oct 24 '17 09:10 ainopara

Thanks for the review tips.

jpsim avatar Oct 24 '17 17:10 jpsim

Can you please include the script used to generate SourceKitDef.swift? Ideally that'd be a command in the Makefile.

jpsim avatar Nov 02 '17 23:11 jpsim

Sure. I'm not sure where to place those template files (including the ProtocolUIDs.def copied from SourceKit project). $(project_root)/Templates or $(project_root)/Defines or any other idea?

ainopara avatar Nov 03 '17 04:11 ainopara

It seems Circle CI was trying to build target sourcekitten for iPhone Simulator then failed.

ainopara avatar Nov 03 '17 04:11 ainopara

Oh, the config file is in version 2 format. We should move it to .circleci/config.yml as https://circleci.com/docs/2.0/migrating-from-1-2/ described.

ainopara avatar Nov 03 '17 04:11 ainopara

I realized what's wrong with this PR. It does not contain a config file for Circle CI. I should rebase on master brunch.

ainopara avatar Nov 03 '17 04:11 ainopara

I am not realize #274 has a similar solution until it is closed. I think my current solution for strong typed keys has some room to improve, compareing with solution from @norio-nomura .

I'm planning to seperate this PR to several small PRs. The first part will be introduction of a redesigned SourceKitDef.swift. I will make a new pull request once it is ready for review.

I will keep this PR open to track what's left to working on.

ainopara avatar Nov 25 '17 14:11 ainopara