Ongoing Minor Issue List (OCD list) about spongeapi HOT 78 CLOSED

OCD is good for those kind of things, haha.

from spongeapi.

Maybe we should cleanup the checked ones?

from spongeapi.

@kenzierocks I considered deleting, but let's keep them there for history. Any new issues will go in a new comment.

EDIT: Github does not track todo items outside of the original issue topic and they're already in the commit history so I'll delete.

from spongeapi.

Creeper.java line 36: "Gets whether or not the creeper has been struck by lightning." shouldn't it be "Gets whether or not the creeper is powered.", considering plugins can power them.

from spongeapi.

@ST-DDT why would we use a nullable when we could use an optional with an overloaded helper?

from spongeapi.

i.e. two methods
void setCarriedBlock(BlockState carriedBlock);
and
void setCarriedBlock(Optional<BlockState> carriedBlock);

or even just the second one.

from spongeapi.

Because null can still fit into both, Optionals are designed for return types.

http://java.dzone.com/articles/java-8-optional-how-use-it

What is Optional not trying to solve
Optional is not meant to be a mechanism to avoid all types of null pointers. The mandatory input parameters of methods and constructors still have to be tested for example.

Like when using null, Optional does not help with conveying the meaning of an absent value. In a similar way that null can mean many different things (value not found, etc.), so can an absent Optional value.

from spongeapi.

@ryantheleach I haven't seen any official Oracle statements that have said or implied that Optionals are only for return types, and the Optional used here is not the Java optional anyway. In my opinion moving the possibly of a missing value into the type system is beneficial in many situations, not just for method return statements.

from spongeapi.

I'm offering this up as an opinion and I hope there is some truth to it.

TL;DR

In Java we're left with either consuming a result that is either wrapped in some class (eg., the Optional idiom) or else testing a return value specifically (eg., null).

NTL;R

The way I'd suggest looking at this is to treat the SpongeAPI as a black box.

When we use Optional<T> as a return type, the Sponge through the API is providing the plugin developer more than a simple assurance that whatever the result of the method is, the result is safe. The result is required to be tested by the caller in a safe manner.

The example is:

public Optional<T> getSomethingFromSponge();

// ...
// and in the caller:

Optional<T> result = getSomethingFromSponge();

if ( result.isAbsent() ) {
   // proceed down sad path
} else {
   // proceed down happy path
}

vs.

public T getSomethingFromSponge()


// ...
// and in the caller:

T result = getSomethingFromSponge();

if ( result == null ) {
   // proceed down sad path ..  Gee, I think.does null mean it truly failed?
} else {
   // proceed down happy path .. Hmm, a non null value  may mean success, perhaps?
}

I'd rather see the use of Optional in the general case because that API signature conveys to the developer relative safety in the result value. It's harder (much harder) to accidently NPE with a result.

On the other hand, when we look at arguments to methods:

doAnotherThing( T  arg)

I'd rather there be no Optional in the API for arguments. The main reason (going back to the black-box metaphor) is Optional arguments to methods implies that the caller knows that the method may not need the argument. That insider knowledge of the implementation is worrisome.

Methods in the Sponge are (or will be) well crafted and do the right amount of due diligence on the arguments passed to them. Using Optional arguments puts the onus on the plugin author to wrap all their calls to Sponge methods with extra protection just in case Sponge isn't careful with the arguments. I can see why some may be skeptical of any new API implementation, but this is an extreme amount of caution.

Another case for methods with Optional arguments comes up when the Sponge tries to deal with Magic Numbers. These magic numbers are the values used internally to represent meaning or behavior. The magic numbers are not meant ever to be known to the caller.

Optional<T>  getSomethingFromSponge( int  arg ) {

   // ...
   if (arg <=  0) {
      // resort to some default behavior 
      this.internalParameter = -1;
   } else {
       // the argument has quality we can use
       this.internalParameter = arg;
   }

   // return something ...
}

The problem with this kind of code IMHO is that the caller must know in advance that when arg is less or equal to 0, that the internal behavior of the class has a default case. A case where perhaps the behavior in that area is turned off or disabled.

So thus, the argument for Optional in the method parameters would suggest:

public Optional<T> getSomething( Optional<Integer> arg ) {
     // ...
     if ( arg.isAbsent() ) {
         // setup behavior to cope with sad-path
     } else {
         // setup behavior to cope with happy path
     }

     // ...
     // return something ...
}

I'd counter that this excuse for Optional is overlooking that the API is simply missing the method:

public Optional<T> getSomething()

Why pass in an Optional that is known by the caller to be literally absent. The caller would have to explicitly make it so.

I cannot make the blanket statement for Sponge that methods shall not have Optional arguments, but I find their use to be specialized.

I could make a blanket statement for Sponge API design that recommends Optional for the return of objects especially when the attempt to produce the result has a non-zero probability of failing while not killing the server with an unnecessary NPE.

All of this is syntax sugar to overcome the fact that Java is merely pass-by-value. There is simply no way to do what we do with pointers in C/C++:

RESULT  doSomething(  int x,   T* pArg ) {
      // whatever the case may be choose:
      //  A)  modify the whatever pArg points to and then return S_OK    or
      //   B) leave pArg alone and return E_FAIL;
}

// and in the caller:

if  ( SUCCEEDED ( doSomething ( n, pFoo ) )  ) {
    // proceed down happy path
} else {
    // proceed down sad path
}

from spongeapi.

There is, since objects can be passed in and their reference is passed by value, you can just modify the object passed in and return a result. (of course this changes if you are passing in a primitive)

Result method(int x, Object pArg){
    //A) Modify the fields of pArg or call methods as appropriate, then return Result.OK or
    //B) leave pArg alone and return Result.FAIL
}

if(Results.success(doSomething(n, pFoo)){
    //proceed down happy path
} else {
    // proceed down sad path
}

But the reason why I am against using Optionals in parameters is

1. what does the value being absent mean? if you want the default, why not pass in PickupTime.default, then you can use PickupTime.Infinite or others if you need further context.
2. if you don't need extra's why not just pass in null? sponge implementations should be checking for it anyway, so using a nullable parameter and labelling it such isn't that bad. Or just have multiple methods, setPickupTimeInfinite() setPickupTimeDefault() setPickupTime(int i)

from spongeapi.

I think (?) we're in agreement that Optional for method arguments is a specialized case.

Where we may (?) agree is using the Optional idiom for return values in the general case.

Thanks

from spongeapi.

This list can now be cleared, to make way for any new minor issues discovered :)

from spongeapi.

@simon816 Have you fixed all of them in your pull request?

from spongeapi.

@Minecrell Yes, that was the main purpose of the PR. Of course you could check by hand if you wanted...

from spongeapi.

@simon816 That's why I asked, I've updated the issue now.

from spongeapi.

I personally can't hit format on GameRegistry because all of the javadoc comments are incorrectly formatted according to Eclipse + Google style formatter. Possibly something to look into?

from spongeapi.

Add @SuppressWarnings("rawtypes") to SimpleServiceManager line 86.

from spongeapi.

Missing 'is' at javadoc in MinecraftVersion:49

from spongeapi.

@Bammerbom Fixed, thanks

from spongeapi.

• BiomeType needs a getter for its name
• Living needs a class javadoc
• PotionEffect#apply is questionable and should possibly be moved to Living

from spongeapi.

Suggested changes from #462

• Missing an EntityUnleashEvent to match our EntityLeashEvent
• getFlamable and setFlamable in ExplosionPrimeEvent are misspelled
• Add potion related events
• EntityInteractEvent and children should implement Cancellable
• EntityTameEvent offers no way either to get the tamer
• EntityTeleportEvent has no velocity methods (for maintaining or changing the velocity of the player after the teleport)
• EntityIgniteEvent getter and setter for fire ticks
• EntityEnterPortalEvent and EntityLeavePortalEvent

from spongeapi.

Missing Javadocs for

• HumanPickUpItemEvent
• PlayerItemConsumeEvent
• PlayerPickUpItemEvent
• LivingPickUpItemEvent
• InventoryClickEvent
• ItemExtractEvent
• RconLoginEvent
• ChunkForcedEvent
• ChunkUnforcedEvent
• AbstractEvent
• ExperienceEvent
• Inventory
• Enchantment

Missing additional methods in:

• HumanEquipmentChangeEvent (Needs getInventory or something)
• PlayerEquipmentChangeEvent ( ditto)
• LivingEquipmentChangeEvent

from spongeapi.

• JavaDocs need improving on Entity#setRotation(Vector3f) and Entity#setLocationAndRotation(Location, Vector3f, EnumSet<RelativePositions>) to specify that the vector is formed as {yaw, pitch, roll}
• Entity needs an overload of setLocationAndRotation that doesn't include the relative position flags

from spongeapi.

• SpongeAPI should use double component based vectors where floats are used as MC uses doubles (it being the target of most implementations). This also means that plugins can give the best precision, an implementation that uses floats can simply reduce it.
• SpongeAPI should use quaternions for rotations (provided by flow-math) as they are superior to euler angles and the standard in games. Both options should be provided: what the implementation doesn't support becomes a simple overload (conversions representations are provided by flow-math).

from spongeapi.

Looks like I made a slight error in my last PR. This doc has the number 35 twice.

from spongeapi.

Here is a list of all classes/interfaces that have missing class javadocs. Most of the Events have already been detected by gabizou, but i added them for completeness.

EDIT: Fixed by gabizou

Shall i help adding them?

from spongeapi.

Tamer.getName returns null, not Optional<String>. Tamer.java

from spongeapi.

#509 - Fixing a typo ("vaildate" --> "validate") in ItemStack.java at line 156.

from spongeapi.

@ST-DDT By default, someone from staff will take care of the outstanding issues on the OCD list. It'll be taken care of soon.

from spongeapi.

@gabizou Thanks for fixing all those javadoc issues:

I scanned the API for more missing javadocs, and this classes are missing their class javadocs.

• org.spongepowered.api.attribute.AttributeCalculator
• org.spongepowered.api.entity.Tamer
• org.spongepowered.api.event.inventory.ItemExtractEvent
• org.spongepowered.api.item.CookedFishes
• org.spongepowered.api.item.DyeColor
• org.spongepowered.api.item.DyeColors
• org.spongepowered.api.item.Enchantment
• org.spongepowered.api.item.Enchantments
• org.spongepowered.api.item.Fishes
• org.spongepowered.api.item.GoldenApples

In addition to that ItemExtractEvent does not extend Event at all.
Was that file commited accidentally?

EDIT: Everything fixed

from spongeapi.

• Remove drops from PlayerDeathEvent into new PlayerDropsEvent
• Make PlayerDeathEvent not cancellable
• Fix spelling of DisgusedBlockData
• Add missing class javadocs for data types and manipulators

@gabizou

from spongeapi.

GenericArguments.none() should return a static CommandElement instead of rebuilding it every time.

from spongeapi.

• Secondary ordinal or Secondary cardinal? Javadoc and field name conflict:

  SpongeAPI/src/main/java/org/spongepowered/api/util/Direction.java

  Line 30 in 20d434c

  * Represent the 16 main and secondary cardinal directions plus up and down.

from spongeapi.

• GameMode javadoc uses fully qualified link to Player. 😞

from spongeapi.

• Now that doclint is turned off, there is no need for unnecessary imports like the ones here

from spongeapi.

• InventoryOperationResult.Type.Cancelled has incomplete javadoc

from spongeapi.

• Ban.User#getUser wrong return type:
  https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/util/ban/Ban.java#L91
  maybe rename to UserBan?

from spongeapi.

Or use the full package declaration.

from spongeapi.

* <li>{@link #NORTH} targeting towards -Z</li>
* <li>{@link #EAST}  targeting towards +X</li>
* <li>{@link #SOUTH} targeting towards +Z</li>
* <li>{@link #WEST}  targeting towards -X</li>
* <li>{@link #UP}    targeting towards +Y</li>
* <li>{@link #DOWN}  targeting towards -Y</li>

• Incorrect JavaDoc for Direction.java #659
  https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/util/Direction.java#L33

from spongeapi.

https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/util/event/factory/EventFactoryPlugin.java

Throws a few checkstyle warning because it uses @param tags in the description. Should be replaced with @code tags.

from spongeapi.

Some method declarations in DataHolder are generic although they don't have to. This leads to worse usability / more raw casts.

DataHolder holder = null;
DataManipulator<?> data = null;
holder.offer(data); // Does not compile
holder.offer((DataManipulator) data);

That offer is usually used in for each loops like SpongeItemStackBuilder.

• <T extends DataManipulator<T>> boolean remove(Class<T> manipulatorClass)
• <T extends DataManipulator<T>> boolean isCompatible(Class<T> manipulatorClass)
• <T extends DataManipulator<T>> DataTransactionResult offer(T manipulatorData)
• <T extends DataManipulator<T>> DataTransactionResult offer(T manipulatorData, DataPriority priority)

They should be declared this way:

• boolean remove(Class<? extends DataManipulator<?>> manipulatorClass)
• boolean isCompatible(Class<? extends DataManipulator<?>> manipulatorClass)
• DataTransactionResult offer(DataManipulator<?> manipulatorData)
• DataTransactionResult offer(DataManipulator<?> manipulatorData, DataPriority priority)

or that way:

• <T extends DataManipulator<?>> boolean remove(Class<T> manipulatorClass)
• <T extends DataManipulator<?>> boolean isCompatible(Class<T> manipulatorClass)
• <T extends DataManipulator<?>> DataTransactionResult offer(T manipulatorData)
• <T extends DataManipulator<?>> DataTransactionResult offer(T manipulatorData, DataPriority priority)

from spongeapi.

Documentation for Direction#getClosest(Vector3d) is incorrect

from spongeapi.

Clean up some javadocs for MessageSinks#combined and MessageSinkFactory#combined: #727

from spongeapi.

https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/service/permission/context/Context.java#L44

Should these be keys or types? The constructor and method calls them types but the public fields call them keys.

from spongeapi.

Quote from @AlphaModder
Now that doclint is turned off, there is no need for unnecessary imports like the ones here

IMO they are still needed. I think they are used when generating the javadoc HTML in order to provide links to the docs for that class.

Quote from @Saladoc
InventoryOperationResult.Type.Cancelled has incomplete javadoc

What's incomplete? There have been no changes since you wrote that comment and to me it reads fine.

from spongeapi.

• DisplacementProperty has no javadocs.
• PlayerResourcePackStatusEvent is in the wrong package, should be in org.spongepowered.api.event.entity.player
• GameMode feels lonely and would like to be moved to the org.spongepowered.api.data.type package.
• ProjectileSource has none Optional return values. Although spawning of the provided Projectile class may not be possible at all times. Such as in UnknownProjectileSource, which itself could/should be a singleton for easier comparison.
• AbstractInventoryProperty should extend AbstractProperty instead of implementing everything twice.
  • As far as i can see the content is exactly the same except these three points:
    • protected Constructor(@nullable V value) implementation this(...) call is flipped (bug?)
    • Constructor Operator parameter being (not) @Nullable (bug?)
    • Class check in equals(Object) is different of course (but keeping the simple overwrite is enough, although i'm not sure whether this is even necessary)

from spongeapi.

Coerce.toLong Number.shortValue() should be Number.longValue().

from spongeapi.

• CommandFlags:162 - [fallthrough] possible fall-through into case
  • last fix did not fix it for my eclipse
  • should use/add @SuppressWarnings("fallthrough")
• PermissionDescription:46 - Indention depth 5 should be 4.
• PermissionDescription:115 - Forbidden summary fragment (What is that/there?).

from spongeapi.

@ST-DDT

CommandFlags:162 - [fallthrough] possible fall-through into case

I think what's wrong is that there is a space between // and $.
Now: // $FALL-THROUGH$
Should be: //$FALL-THROUGH$

from spongeapi.

• [ ]

https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/text/Texts.java#L467

Javadoc states it takes a color to strip as opposed to a format marker. Implementation instead looks for a formatter marker and removes both the marker and the color.

https://github.com/SpongePowered/SpongeCommon/blob/master/src/main/java/org/spongepowered/common/text/LegacyTextRepresentation.java#L201

Either the JavaDocs should be fixed, or the implementation should be changed to match the javadocs.

from spongeapi.

GroundLumanceProperty is misspelled (should be GroundLuminanceProperty), and LightEmissionProperty is inconsistently named (it would make more sense to call it LuminanceProperty to match GroundLuminanceProperty and SkyLuminanceProperty.

Also, there should probably be a HumidityProperty for Locations.

from spongeapi.

• Typo fro instead of forin ExperienceHolderData
• Unused import ImmutableDataManipulator in ImmutableCareerData

from spongeapi.

• Inventory.slots() should have a signature of <T extends Slot> Iterable<T> slots(); because it always returns Slots.
  @Mumfrey

from spongeapi.

• Location and Transform use setXXX methods for creating new instances with modified data.
  BlockSnapshot uses withXXX methods. As these methods don't set something but create a new instance, withXXX seems more appropriate here. Needs to be renamed.

from spongeapi.

• Keys - missing ACHIEVEMENTS key

+ public static final Key<SetValue<Achievement>> ACHIEVEMENTS = null;

• ImmutableAchievementData - typo

- ImmutableSetValue<Achievement> achiements();
+ ImmutableSetValue<Achievement> achievements();

I can create a PR for this as well, but I consider this a too small/OCD change. Compare/Merge

from spongeapi.

• PersistingData - typo

- Value<Boolean> perists();
+ Value<Boolean> persists();

from spongeapi.

Inventory.slots() should have a signature of Iterable slots(); because it always returns Slots. @Mumfrey

As mentioned here this is a big fat no:

Leaf nodes are currently guaranteed to be Slots but I definitely don't want this encoded in the API, because the entire point of this is that it can support future changes in minecraft's inventory system. This is why pretty much all generic signatures are , because the contract is designed around the core premise that you deal with Inventorys not any particular sub interface.

• Mumfrey 2015

from spongeapi.

• Did blockbuffers get removed / renamed from the API? If so https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/world/gen/WorldGenerator.java needs it's JavaDocs updated.

from spongeapi.

• I wouldn't call this a minor issue, but maybe someone else would: #895 Helper method returning wrong Axis

from spongeapi.

• In SleepingEvent change the #wasSpawnSet() into #isSpawnSet() in the JavaDocs

from spongeapi.

SPAWNER_REQURED_PLAYER_RANGE - typo

from spongeapi.

I already fixed that in #860.

from spongeapi.

https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/entity/living/monster/Creeper.java
line 30 creeepr -> creeper

from spongeapi.

That was also already fixed in my PR.

from spongeapi.

@bloodmc Might need discussion, but things related to BlockTrait values (like getTraitMap) should return ? extends Comparable since all values are Comparable.

For example:

// Current
Map<BlockTrait<?>, ?> getTraitMap();

// Proposed
Map<BlockTrait<?>, ? extends Comparable<?>> getTraitMap();
// or
Map<BlockTrait<?>, Comparable<?>> getTraitMap();

from spongeapi.

Inventory.poll() returns ItemStack, but the javadoc claims it can return Optional.empty()! link to relevant line (Fixed)

from spongeapi.

@phroa mind updating the issue description of the various OCD things listed here?

from spongeapi.

• Create Keys.BABY (boolean) and Keys.Adult (boolean)

from spongeapi.

@ryantheleach I think only one would be needed - the value of one is simply the opposite of the other.

from spongeapi.

• Add Enderman Carried Data to api. Different from equipment / loot.

from spongeapi.

• Scoreboard.java: line 58, blank line missing.

from spongeapi.

• Keys.EXPLOSIVE_RADIUS, ExplosiveRadiusData and Explosive are inconsistent in whether the explosive radius is a Value<Integer> or a MutableBoundedValue<Integer>. Should be the latter because negative radius makes no sense.

from spongeapi.

• ImmutableFlamableData is misspelled

from spongeapi.

• Sink package is left over after rename to channels: https://github.com/SpongePowered/SpongeAPI/tree/master/src/main/java/org/spongepowered/api/text/sink

from spongeapi.

@ryantheleach feature requests aren't minor changes, those should be an issue.

from spongeapi.

Creating a new OCD list for beta See #1000

Any still unresolved issues from here that I may have missed should be reported there.

from spongeapi.

@Saladoc remake your concern as an issue as well

from spongeapi.

https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/entity/EntityTypes.java#L64 shouldn't it be ZOMBIE_PIGMAN instead of PIG_ZOMBIE?

from spongeapi.