Skip to content
This repository was archived by the owner on Dec 18, 2023. It is now read-only.

Add support for topologies. #44

Merged
merged 1 commit into from
Nov 30, 2017
Merged

Add support for topologies. #44

merged 1 commit into from
Nov 30, 2017

Conversation

srmurguia
Copy link
Contributor

Now it is possible to add dependencies by specifying topologies. At this
moment only FULL, RANDOM and LINEAR are implemented. Once this change is
reviewed and accepted more types will be added.

See diff on AndroidStudioPoet.kt for an example of a random topology.

Now it is possible to add dependencies by specifying topologies. At this
moment only FULL, RANDOM and LINEAR are implemented. Once this change is
reviewed and accepted more types will be added.

See diff on AndroidStudioPoet.kt for an example of a random topology.
@NikitaKozlov
Copy link
Contributor

Thank you very much for this PR! It looks awesome, but I have a concern in relation to the new format. I didn't take into account topologies, so there is a restriction that modules from one "bulk" (aka ModuleConfig) can't reference each other. Mb you have some ideas how can we support topologies in the new format as well? I do understand that it is not a part of this PR, so we can move this discussion where it belongs to. In general, I don't have anything against merging, code LGTM 👍.

@srmurguia
Copy link
Contributor Author

I think we can handle this by adding explicit dependencies outside ModuleConfig, I will elaborate more in a comment on #36

@srmurguia srmurguia closed this Nov 29, 2017
@srmurguia srmurguia reopened this Nov 29, 2017
@srmurguia
Copy link
Contributor Author

Github is hard! Wanted to comment but hit close and comment :p

@NikitaKozlov
Copy link
Contributor

NikitaKozlov commented Nov 29, 2017

I just got a different idea about new input. Lets put it inside the ModuleConfig, but the rule would be that if ModuleConfig has topography then its "dependency" section is ignored. Will that work for you? If we put it outside of the ConfigModule, it might be hard to be sure that generated project is always buildable, for example, because android application module can't depend on another android application module.

@srmurguia
Copy link
Contributor Author

Sorry for the confusion, I do not want to move the dependency section outside ModuleConfig, I like it where it is since it reflects the structure of the build files. Topologies (or topographies, actually I am not sure what is a better name) on the other hand has to do with the whole project, that is why I think they should be outside ModuleConfig.

Those rules (App not depending in other app, etc.) should be enforced by the implementation of each topology. Current implementation does not take that in consideration since all the modules are java libraries but once we have apps, android libraries and java libraries, the implementation should be done such that:

  • Java libraries only depend on java libraries
  • Android libraries only depend on other android and java libraries
  • Android apps do not depend on other android apps

@NikitaKozlov
Copy link
Contributor

Do you think we should respect "dependency" sections or ignore? In order to avoid circular dependencies, it might be a good idea to ignore them.

@srmurguia
Copy link
Contributor Author

I think we should respect them and add something that causes a warning when circular dependencies are generated. There are cases where circular dependencies are valid (for example, when using testimplementation dependencies, see https://issuetracker.google.com/63897699)

@NikitaKozlov
Copy link
Contributor

Ok, then let's merge this thing as it is, then we can decide how do we implement support for it in the new format.

@borisf borisf merged commit 495e888 into android:master Nov 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants