Add assets attribute - Githubissues

windy1 commented 8 years ago

Update for plugin-meta changes

Signed-off-by: Walker Crouse walkercrouse@hotmail.com

stephan-gh commented 8 years ago

The plugin-meta repo was separate on purpose, you shouldn't merge it in here because otherwise you will either a) merge the whole SpongeGradle plugin into the SpongeVanilla JAR which is obviously stupid, or b) exclude classes manually which is not nice either.

Also you haven't moved theNOTICE file so the Apache license of the Maven version range parser is no longer properly attributed. Any reason you merge this without giving any chance to review this? There was a reason everything was the way it was.

Pulling in a Gradle plugin in SpongeAPI is not a good idea and can easily break in the future once Gradle changes something and includes the Groovy dependency in the Maven POM for example.

windy1 commented 8 years ago

IMO it's not such a big deal if the other classes of SpongeGradle are included since it's pretty small and has no external dependencies. Also, it wasn't my idea to merge these @Mumfrey and @gabizou both thought the projects should be merged so I just did it while I was merging the AssetManager since the only reason I created a bajillion PRs for it was because I couldn't push to SpongeGradle or plugin-meta previously.

stephan-gh commented 8 years ago

@windy1 Independent of the choice whether plugin-meta should be in here or not, I see no point in creating pull requests if you merge them after an hour. When I made the whole plugin-meta system I didn't simply put everything together randomly, I've thought about how to structure it and also already considered future additions to the format like this is one.

Considering nothing was really waiting on this I see no point of merging this after an hour without giving others the chance to share their thoughts or give feedback. What would have changed if you merged this 24 hours later instead of after an hour? Allowing people to review changes is the whole point of pull requests and is important in projects that grow bigger and bigger so I personally don't really see the point in opening the pull request at all in that case.

Anyway, I completely agree with adding something for the assets, however I'd have mentioned a few things to keep in mind for the implementation:

For additional (unofficial) properties for the mcmod.info format (not supported by Forge) I designed the plugin-meta serializer to have a concept I call extensions, which is essentially a map where everything goes in that is unknown to the main part of the serialization (the options supported by both Forge and our serializer). The reason I have designed it like this is that it prevents future conflicts if Forge would decide to add an assets properly that works differently.

Extensions are basically extra classes (or even just a custom JSON deserializer) that can be registered when constructing the mcmod.info serializer. For example, you would use it in the following way:
```
public class SpongeExtension {
  private String assets;

  // ...
}

McModInfo serializer = McModInfo.builder().registerExtension("sponge", SpongeExtension.class).build();

PluginMetadata metadata = new PluginMetadata("testplugin");

SpongeExtension sponge = new SpongeExtension();
sponge.setAssetDirectory("foo");
metadata.setExtension("sponge", sponge);

serializer.write(Paths.get("mcmod.info"));
PluginMetadata readMetadata = serializer.read(Paths.get("mcmod.info"));

SpongeExtension readSponge = readMetadata.getExtension("sponge");
```
With no required changes for the plugin-meta repository this would result in the following:
```
[{
  "modid": "testplugin",
  "sponge": {
      "assets": "foo"
  }
}]
```
The whole extension system is already included but still a bit WIP because I didn't need it yet. The design of this API also shows the primary reason why it was kept in a separate repository: The plugin-meta serializer is not Sponge-specific nor was it ever designed to be. I kept it in a separate repository, so other applications (launchers, a custom Plugin portal, ...) could use it for any mod/plugin using a mcmod.info file.

I did agree with moving the Gradle plugin parts to this repository because it makes sense to put all Gradle portions of the Sponge project together in one repository and they're still quite closely related to each other. Gradle's plugin design allows a single dependency to define multiple Gradle plugins independently, so even though they're still in the same repository they're still separate plugins that need to be applied independent from each other.

The plugin meta serializer on the other hand is neither related to Gradle nor is it directly related to Sponge plugins, it was always supposed to be independent and should be kept in a separate repository for the same reason you wouldn't put Guava and SpongeGradle together, they can be used (and useful!) completely independently.
The SpongeGradle repository is now missing the dependencies on Guava and GSON. These are available in development environment and at compile time because Gradle bundles them, however they're only actually applied to the classpath in production if one of the internal Gradle parts requests them.

They will need to be added back or we will run run into missing classes at runtime in certain situations where the dependencies have not been brought in by one of Gradle's internals yet.

windy1 commented 8 years ago

I only opened the PR because I didn't have push access to SpongeGradle, I am allowed to contribute to the code base without opening a PR... Honestly I kind of feel like you're making mountains of molehills here. I can refactor the implementations to use extensions a little later, but as far as I can tell there's absolutely no reason to keep plugin-meta independent from SpongeGradle. I've tested the implementation and as far as I can tell there are no classpath errors associated with it.

kashike commented 8 years ago

I agree with @Minecrell here, and I had actually mentioned that plugin-meta was more than likely separate so that other projects could utilise it. You gave no one time to look over these PRs, just like with https://github.com/SpongePowered/SpongeCommon/pull/552.

windy1 commented 8 years ago

The decision to merge the projects did not come from me @kashike and if I created a PR and waited for each one to be reviewed my workflow would be slowed significantly. Fact of the matter is I have push access so I intend to use it.

kashike commented 8 years ago

I get that you have push access, but it would still be nice if you were to give people time to look at/comment on things. Another case is https://github.com/SpongePowered/SpongeAPI/pull/1118#issuecomment-189977819 - you say you want to get it merged, and then you do so nearly 2 hours later ignoring the existing comments and not allowing new ones to come up (especially annoying when comments are spread throughout ~5 PRs). In the case of https://github.com/SpongePowered/SpongeCommon/pull/552, you call me nitpicky even though it was intentionally changed to use star imports.

Zidane commented 8 years ago

@windy1

Lets go ahead and keep the plugin-meta portion OUT of this repo but keep everything else in.

@ Everyone

It wasn't windy's decision to merge this together. It was made in error to merge this with everything (without realizing minecrell's intent). No reason to jump at each other, it'll get resolved shortly. If you want someone to rage at, rage at me :P.

windy1 commented 8 years ago

I'll just say one last thing. @kashike I didn't ignore those comments, I disagreed with them and decided to go ahead with the merge. I am always open to constructive criticism but im not sure why you have this notion that once a PR is merged that it is set in stone. Things are subject to change, humans oversee things sometimes not to mention 90% of your comments are purely semantical and have nothing to do with the actual content. I'll just leave it at that.

SpongePowered / SpongeGradle

Add assets attribute #3