search

JamitLabs / Accio

A dependency manager driven by SwiftPM that works for iOS/tvOS/watchOS/macOS projects.
MIT License
664 stars 32 forks source link

Pass options to Accio as comments in the Package.swift file, 2nd implementation proposal #74

Closed acecilia closed 5 years ago

acecilia commented 5 years ago

This PR contains a new implementation proposal based on the discussion in https://github.com/JamitLabs/Accio/pull/55

Jeehut commented 5 years ago

@acecilia Thank you for the updated request. Will have a detailed look when I have more time, but looks good overall when skipping through it. Just one question: How are the new available options being used during the build process? I couldn't see where the new service is being used as of now ...

acecilia commented 5 years ago

@Dschee

How are the new available options being used during the build process?

Thats the golden question... When I tried to implement it, it was not that easy: I could not find a unique place to consume the service. I will give you an example: think about implementing the source value for the integration option. The requirements are as follows:

At the moment, after resolving the dependency tree and building the frameworks, the only information that is passed to Step2 (in which the integration in the main Xcode project happens) is an array of FrameworkProduct. Such information is not enough for the Xcode integration step to do the right thing when the dependency is integrated using code.

I found a similar problem when thinking about the other options: a bigger refactor is needed to accommodate them, is not just about an additive change to the source code.

As a result, I think is better to keep this PR small, and refactor the code as the new options (source, static...) are introduced.

Makes sense?

Jeehut commented 5 years ago

I basically agree on all points. I just think we should not merge this into the stable branch until at least one user-facing feature is using the new service. If there's any downside with this suggestion, I'm perfectly fine with creating a separate branch where you can merge the service already, e.g. if you wanted to keep each feature in a separate PR. But maybe it's enough if you just add the first API into this PR and we merge it then?

acecilia commented 5 years ago

Super. Yes please, can you create a branch? And I think I can easily update my existing PR adding cocoapods integration, and merge it also in this new branch. And when you feel is ready you can move it to stable.

Jeehut commented 5 years ago

@acecilia I just gave you contributor rights, so you should now be able to create branches yourself. How about that? :)

acecilia commented 5 years ago

🔥

acecilia commented 5 years ago

@Dschee I am not being able to create branches, and I am not sure if I am doing something wrong. I am trying to push a new branch to the repo with the command git -c credential.helper= push -u origin work/additional-parameters-as-comments (the credential helper thing comes from https://stackoverflow.com/a/44033298) and putting my github user and password in the command line prompt, but this is the result:

Pushing to https://github.com/JamitLabs/Accio.git
remote: Permission to JamitLabs/Accio.git denied to acecilia.
fatal: unable to access 'https://github.com/JamitLabs/Accio.git/': The requested URL returned error: 403
acecilia commented 5 years ago

Oh wait, let me try again, I just received the invitation email

acecilia commented 5 years ago

Working now, thanks @Dschee Would you like to review each PR to the new branch work/additional-parameters-as-comments, or should I open just one PR when I consider is ready to go to stable?

Jeehut commented 5 years ago

Regarding git help, I personally use https://git-fork.com/ as a GUI and can only recommend it. Amazing software and it's free, too. Also, I'm using the recommended way via SSH keys for authentication. See here for additional details. Maybe you want to try that, too.

Having that said, regarding the PR & branches, here's how I meant it: You can open as many work/... branches as you wish, no need for PRs there. But I recommend you to define a branch, maybe something like work/manifest-improvements, and put your current basic logic in there (without a PR, just push your renamed branch). Then, you can work on specific uses of this basic logic and post a PR targeting your work branch. This way, we can review one or multiple usages of the basic logic and merge them one by one once they are ready. After at least one usage is merged to your work branch, you can post a PR targeting the stable branch with your work branch. Then, I think, we can review the thing as a whole and merge it to stable and continue on from there.

I hope I could clarify the process. Feel free to ask me any questions if I missed something.