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 support for enabling/disabling synthesizing interfaces for resources in a per-project basis #2249

Closed
pepicrft opened this issue Jan 6, 2021 · 8 comments
Labels
good first issue Good for newcomers type:enhancement New feature or request

Comments

@pepicrft
Copy link
Contributor

pepicrft commented Jan 6, 2021

Context 🕵️‍♀️

As @sgrgrsn brought up here, it'd be useful to allow disabling/enabling the synthesizing of interfaces for resources on a per-project basis.

What 🌱

We can extend the generation option to support passing a list of projects for which the synthesizing should be disabled/enabled. I think we can deprecate the existing .disableSynthesizedResourceAccessors in favor of something along the lines of:

let config = Config(generationOptions: [.synthesizedResourceAccessors(projects: .all)])
let config = Config(generationOptions: [.synthesizedResourceAccessors(projects: ["A", "B"])])
let config = Config(generationOptions: [.synthesizedResourceAccessors(projects: .except(["A", "B"]))])
@pepicrft pepicrft added type:enhancement New feature or request good first issue Good for newcomers labels Jan 6, 2021
@sgrgrsn
Copy link
Collaborator

sgrgrsn commented Jan 7, 2021

I was thinking if it would be better to specify the setting when defining the projects in the manifests instead of coupling the Config.swift with the actual project names. If we have it as a part of the project descriptions we would also avoid ending up in a situation where the Config enables the synthesizing on a project that no longer exists.
I think the config could then be changed to have the default setting. Something like:

let config = Config(generationOptions: [.synthesizedResourceAccessorsDefault(true)])
let config = Config(generationOptions: [.synthesizedResourceAccessorsDefault(false)])

I hope it makes sense - let me know what you think 👍

@rstone4-chwy
Copy link

If I wanted to take this on as a first time PR, do you have any suggestions on where to start looking or what you want the final API to look like?

@fortmarek
Copy link
Member

fortmarek commented Feb 10, 2021

I was thinking if it would be better to specify the setting when defining the projects in the manifests instead of coupling the Config.swift with the actual project names

I agree with this

let config = Config(generationOptions: [.synthesizedResourceAccessorsDefault(true)])

Maybe we could keep disableSynthesizedResourceAccessors which would translate to .synthesizedResourceAccessorsDefault(false)]? And without this flag it'd default to .synthesizedResourceAccessorsDefault(true).

Did you have @sgrgrsn any specific API for how the Project configuration could look like?

On a different note:
Thanks for the interest @rstone4-chwy!

This is, indeed, a good first issue once we decide on the API. Feel free to come up with eg. the Project configuration of accessors and we can discuss it more here. Aftewards, I'll be happy to guide you further and give you some leads as to where to look. There are also other good issues for new contributors as this one is not completely ready yet. Feel free to hit me up at tuist slack, too

@sgrgrsn
Copy link
Collaborator

sgrgrsn commented Feb 11, 2021

Did you have @sgrgrsn any specific API for how the Project configuration could look like?

Could it be something like this, @fortmarek?

Project(
    name: "Project with resources",
    synthesizedResourceAccessors: true
)

Or do you think we should have something like the generationOptions as in the Config structure?

@fortmarek
Copy link
Member

I believe that makes sense, I agree with this proposal 👍

@pepicrft
Copy link
Contributor Author

I like that this can be defined at a per-project level. I think eventually we should remove those global options in favor of per-project definitions.

@rstone4-chwy
Copy link

I haven't forgotten about this, just been busy. I'll try to get in the slack tonight and see if I can get a start on how this would be setup.

@danieleformichelli
Copy link
Collaborator

Fixed in #3963 getting rid of the Config.disableSynthesizedResourceAccessors and adding it as ProjectDescription.Project.ProjectOption

@danieleformichelli danieleformichelli added this to the 3.0 milestone Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type:enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants