search

elucent / GravelOres

Simple gravel ores for 1.11.2
2 stars 4 forks source link

Some changes #5

Closed Sunconure11 closed 6 years ago

Sunconure11 commented 6 years ago
  1. Travis CI capacity, if desired, the mod maker can now use travis to deploy builds, and test them.
  2. Pruned some workspace junk, and added a stronger gitignore file.
KnightMiner commented 6 years ago

Formatting is something that makes more sense done directly as its easier to keep track of what changed. In any case, I don't see the point in changing the format for the project, keep the old style.

Not sure how nessesscary Travis is with a mod of this size, but I guess it cannot hurt to have.

Sunconure11 commented 6 years ago

I reformatted it because it was a bit... chaotic, in some senses. Simply Tea had the same kind of chaos.

As for Travis, it'd probably be good to have around, yeah.

KnightMiner commented 6 years ago

Chaotic, maybe, but you made some major stylistic changes such as tabs vs spaces and brackets on new lines, neither of which needed to change. In any case, it's not something that makes sense to do in a PR. I'd argue mappings l/Forge version does not make sense in a PR as well, unless something in the PR needs newer mappings or Forge (I typically leave them on my projects until i need new features.

I say separate out the Travis and gitignore stuff and maybe the workspace pruning and I'll look at that by itself.

Sunconure11 commented 6 years ago

Hm, I'll do that, then. I'll set up travis, cleaned up workspace, and the new gitignore in their own pull or something.

Sunconure11 commented 6 years ago

Alright, reformatting has been dealt with. Should I revert the mappings back too? The ones used by Elucent were pretty ancient.

If you have access to Simply Tea, I also reverted my formatting changes there.

Sunconure11 commented 6 years ago

I guess I'll revert the changes to mappings, then.

God why is multi-tasking like peeing shrapnel

Alright, we all good now?

KnightMiner commented 6 years ago

Okay, I'll look at the rest when I get a chance.

Sunconure11 commented 6 years ago

BTW, you're going to have to talk to Elucent once you accept these pulls, mainly, on the regards of the license. That license actually is Forge's, its leftover workspace junk he didn't clean up. However, he puts all of his stuff under the MIT License, and as such, the codebase should be using that license, not the LGPL license that Forge has embedded in its default workspace junk.

Might want to talk to @Tehnut about this too, since he's in charge of the continuation of Embers and Roots. This is a common thing across all of his mods.

TehNut commented 6 years ago

The licensing issue is all technicality in this case.

The Forge license does not apply as the header specifically states Forge, which puts the repo under ARR. However, Elucent has specifically stayed the repo is MIT, at least in my case about Roots and Embers.

Since Knight has been granted permission to the repo, he gets rights as well.

I've fixed the repo formats locally for R and E. Just haven't synced yet.

KnightMiner commented 6 years ago

I had some time and I looked into Travis, and I think its a bit overkill for a project this small. Since commits are rare (you will notice there is still less than 10) and builds typically come right after commits, I don't really see the point in adding an additional site hosting builds rather than just directly uploading to CurseForge.

Since the rest is just cleanup, I think I'll handle that when I do the formatting. Thanks for your patience, and sorry I've been so slow responding.