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

Move Unit from Energy to Core #271

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Move Unit from Energy to Core #271

wants to merge 5 commits into from

Conversation

ExE-Boss
Copy link
Member

This PR will move Unit from Energy to Core.

Reason:

Unit seems like a class that would benefit more from being in Core than in Energy.

Other changes:

UnitDisplay is now immutable, and accepts a DoubleSupplier to allow linking to a getter.
Example:

UnitDisplay display = new UnitDisplay(Unit.JOULE, components.get(EnergyStorage.class)::getEnergy);

Modded Minecraft energy units were left out and will be added by NOVA-Team/NOVA-Energy#3.

@ExE-Boss ExE-Boss requested a review from RX14 February 25, 2017 23:43
@ExE-Boss ExE-Boss self-assigned this Feb 25, 2017
@ExE-Boss ExE-Boss added feature ready This is ready to be worked on labels Feb 25, 2017
@ExE-Boss ExE-Boss added this to the v0.1.0 milestone Feb 25, 2017
@ExE-Boss ExE-Boss added the enhancement Nice to have features label Feb 25, 2017
@codecov-io
Copy link

codecov-io commented Feb 25, 2017

Codecov Report

Merging #271 into master will increase coverage by 0.16%.
The diff coverage is 21.17%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #271      +/-   ##
============================================
+ Coverage     13.58%   13.74%   +0.16%     
- Complexity      732      762      +30     
============================================
  Files           409      413       +4     
  Lines         12788    13010     +222     
  Branches       1354     1380      +26     
============================================
+ Hits           1737     1788      +51     
- Misses        10934    11107     +173     
+ Partials        117      115       -2
Impacted Files Coverage Δ Complexity Δ
.../main/java/nova/core/util/unit/UnitConversion.java 0% <0%> (ø) 0 <0> (?)
src/main/java/nova/core/util/unit/UnitDisplay.java 0% <0%> (ø) 0 <0> (?)
src/main/java/nova/core/util/unit/Unit.java 0% <0%> (ø) 0 <0> (?)
src/main/java/nova/core/util/unit/UnitPrefix.java 100% <100%> (ø) 10 <10> (?)
src/main/java/nova/core/util/math/MathUtil.java 86.17% <81.48%> (+4.08%) 65 <18> (+20) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8daac6...9eda38e. Read the comment docs.

@ExE-Boss ExE-Boss force-pushed the move/unit branch 5 times, most recently from de1de1e to 7504084 Compare February 26, 2017 08:56
RX14
RX14 previously requested changes Mar 11, 2017
public final class Unit implements Identifiable {
private static final Registry<Unit> REGISTRY = new Registry<>();

public static final Unit METRE = getOrCreateUnit("nova:metre", "Meter", "m");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Meter/Metre/ please

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make use of the Translateable interface

@ExE-Boss
Copy link
Member Author

Changes implemented and rebased to master

@ExE-Boss ExE-Boss removed this from the v0.1.0 milestone Mar 13, 2017
@ExE-Boss ExE-Boss added in progress Pull requests that are not yet ready to be merged and issues that are being worked on. and removed ready This is ready to be worked on labels Mar 22, 2017
@ExE-Boss ExE-Boss dismissed RX14’s stale review April 7, 2017 21:32

The requested changes have been addressed

@ExE-Boss ExE-Boss added the rebase needed 🚧 This PR needs to resolve a merge conflict label Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Nice to have features feature in progress Pull requests that are not yet ready to be merged and issues that are being worked on. rebase needed 🚧 This PR needs to resolve a merge conflict
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants