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

[WIP] Refactor Request to make it resilience. #428

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ainopara
Copy link
Contributor

@ainopara ainopara commented Oct 6, 2017

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.

@jpsim
Copy link
Owner

jpsim commented Oct 23, 2017

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

@ainopara
Copy link
Contributor Author

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.

@jpsim
Copy link
Owner

jpsim commented Oct 24, 2017

Thanks for the review tips.

@jpsim
Copy link
Owner

jpsim commented Nov 2, 2017

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

@ainopara
Copy link
Contributor Author

ainopara commented Nov 3, 2017

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
Copy link
Contributor Author

ainopara commented Nov 3, 2017

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

@ainopara
Copy link
Contributor Author

ainopara commented Nov 3, 2017

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
Copy link
Contributor Author

ainopara commented Nov 3, 2017

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 ainopara changed the title Refactor Request to make it resilience. [WIP] Refactor Request to make it resilience. Nov 25, 2017
@ainopara
Copy link
Contributor Author

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.

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.

2 participants