Commit
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -256,7 +256,9 @@ public static World getWorld(UUID uid) { | |
|
|
||
| /** | ||
| * @see Server#getMap(short id) | ||
| * @deprecated Magic value | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong. 
nightpool
via email
|
||
| */ | ||
| @Deprecated | ||
| public static MapView getMap(short id) { | ||
| return server.getMap(id); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,6 @@ | |
| import org.bukkit.material.Crops; | ||
| import org.bukkit.material.DetectorRail; | ||
| import org.bukkit.material.Diode; | ||
| import org.bukkit.material.Dispenser; | ||
| import org.bukkit.material.Door; | ||
| import org.bukkit.material.Dye; | ||
|
|
@@ -451,7 +450,9 @@ private Material(final int id, final int stack, final int durability, final Clas | |
| * Gets the item ID or block ID of this Material | ||
| * | ||
| * @return ID of this material | ||
| * @deprecated Magic value | ||
| */ | ||
| @Deprecated | ||
| public int getId() { | ||
| return id; | ||
| } | ||
|
|
@@ -489,7 +490,9 @@ public Class<? extends MaterialData> getData() { | |
| * | ||
| * @param raw Initial data to construct the MaterialData with | ||
| * @return New MaterialData with the given data | ||
| * @deprecated Magic value | ||
| */ | ||
| @Deprecated | ||
| public MaterialData getNewData(final byte raw) { | ||
| try { | ||
| return ctor.newInstance(id, raw); | ||
|
|
@@ -556,7 +559,9 @@ public boolean isEdible() { | |
| * | ||
| * @param id ID of the material to get | ||
| * @return Material if found, or null | ||
| * @deprecated Magic value | ||
| */ | ||
| @Deprecated | ||
| public static Material getMaterial(final int id) { | ||
This comment has been minimized.
Sorry, something went wrong. 
Black-Hole
Contributor
|
||
| if (byId.length > id && id >= 0) { | ||
| return byId[id]; | ||
|
|
@@ -580,7 +585,9 @@ public static Material getMaterial(final String name) { | |
| /** | ||
| * Attempts to match the Material with the given name. | ||
| * This is a match lookup; names will be converted to uppercase, then stripped | ||
| * of special characters in an attempt to format it like the enum. | ||
| * <p> | ||
| * Using this for match by ID is deprecated. | ||
| * | ||
| * @param name Name of the material to get | ||
| * @return Material if found, or null | ||
|
|
||
32 comments
on commit 1f83111
There was a problem hiding this comment.
Please tell me this isnt just the first step to removing magic values, and just something to defer people from using them.
There was a problem hiding this comment.
@me4502 1f83111#commitcomment-4016864
Please tell me this isnt just the first step to removing magic values, and just something to defer people from using them.
It's a first step, as having them means we can't provide API compatibility.
There was a problem hiding this comment.
API compatibility with what? And I'm also kinda confused about how PotionType getByEffect(PotionEffectType effectType) is a magic value. Also, the MapCanvas API is almost entirely unusable now[1], as well as the MapCursor and I'm sure plenty of other APIs who either don't have enums or don't accept them in their methods or constructors. The most egregious example of this is any data value for a material that doesn't have a more specific type, such as glass or ice. (Vanilla Minecraft supports adding custom textures to data values of arbitrary materials, and glass and ice are two of the most common for that purpose.)
[1]: As in, using only non-deprecated methods and members. None of the actual functionality has changed, obviously.
There was a problem hiding this comment.
@nightpool 1f83111#commitcomment-4017279
API compatibility with what? And I'm also kinda confused about how
PotionType getByEffect(PotionEffectType effectType)is a magic value. Also, the MapCanvas API is almost entirely unusable now[1], as well as the MapCursor and I'm sure plenty of other APIs who either don't have enums or don't accept them in their methods or constructors. The most egregious example of this is any data value for a material that doesn't have a more specific type, such as glass or ice. (Vanilla Minecraft supports adding custom textures to data values of arbitrary materials, and glass and ice are two of the most common for that purpose.)[1]: As in, using only non-deprecated methods and members. None of the actual functionality has changed, obviously.
Compatibility for future versions of Minecraft. Deprecating things also does not make it unusable; it just means we don't know how long it will continue to function and that we need a better means to facilitate the concepts. The lack of better alternatives doesn't make the current system correct.
There was a problem hiding this comment.
Why do we need to deprecate these things right now if there is no current plan to change this API? I now have to add a bunch of stupid @SuppressWarning calls just because of these deprecated functions, which don't currently have alternatives at the moment.
There was a problem hiding this comment.
@Pocketkid2 1f83111#commitcomment-4028348
Why do we need to deprecate these things right now if there is no current plan to change this API? I now have to add a bunch of stupid @SuppressWarning calls just because of these deprecated functions, which don't currently have alternatives at the moment.
"no current plan to change this API" is a straw-man argument. We haven't said we don't plan on changing API. Where the current API is insufficient, we need to add appropriate API.
I also recommend against adding @SuppressWarning annotations because most of the current calls actually do have real alternatives.
There was a problem hiding this comment.
Oh and by the way, what is a magic value?
There was a problem hiding this comment.
What I mean by "current plan" was a solid plan for when to change the API and how exactly it will happen. As I recall, you only briefly talked about when you planned to make these changes and how exactly. For the record, the API calls I am using don't seem to have any sensible alternatives.
There was a problem hiding this comment.
A "magic number", in this context, is a number used in coding that has unexplained meaning and should be replaced by a variable. This changeset is meant to discourage the use of magic numbers in Bukkit coding. However, many of the values it deprecates are not actually prone to be used with magic numbers, for example the Map API, which has constants that you should use for its arguments. I fail to see why any of those functions or values should have been marked deprecated, and IMO, this commit was written with either an immense lack of consideration for what was actually changed, or with some sort of automated tool left to run rampant.
By the way, @Wolvereness, when you say "we", who exactly are you referring to? Is there some sort of secret "true bukkit developers" chat room where you guys make all the decisions for the community without discussing it with anyone[1]? This has been a disturbing trend I've seen in Bukkit development, a lack of transparency or basic communication, and I would be very grateful to be pointed to something I've missed, some sort of roadmap or discussion forum where these things can be discussed ahead of time.
[1]: I don't actually believe this, its just hyperbole to help make my point. I'm not a conspiracy theorist, I just want to start seeing a little more transparency in Bukkit's planning processes.
There was a problem hiding this comment.
@nightpool 1f83111#commitcomment-4028554
A "magic number", in this context, is a number used in coding that has unexplained meaning and should be replaced by a variable. This changeset is meant to discourage the use of magic numbers in Bukkit coding. However, many of the values it deprecates are not actually prone to be used with magic numbers, for example the Map API, which has constants that you should use for its arguments. I fail to see why any of those functions or values should have been marked deprecated, and IMO, this commit was written with either an immense lack of consideration for what was actually changed, or with some sort of automated tool left to run rampant.
By the way, @Wolvereness, when you say "we", who exactly are you referring to? Is there some sort of secret "true bukkit developers" chat room where you guys make all the decisions for the community without discussing it with anyone[1]? This has been a disturbing trend I've seen in Bukkit development, a lack of transparency or basic communication, and I would be very grateful to be pointed to something I've missed, some sort of roadmap or discussion forum where these things can be discussed ahead of time.
[1]: I don't actually believe this, its just hyperbole to help make my point. I'm not a conspiracy theorist, I just want to start seeing a little more transparency in Bukkit's planning processes.
The concept that certain maps are in some pool with a limit of Short.MAX_VALUE can easily be interpreted as an oversight on the part of the original implementation. The magic values, in reference to this commit, are values that we have no way to actually guaranty to plugins. If the value changes meaning, there is no way to update Bukkit / CraftBukkit to have them be non-breaking.
Which, brings the next point, "we" is a reference to the individuals responsible for updating CraftBukkit, A.K.A. the Bukkit Team. Changes are always subject to community discussion, but not subject to a vote. Individual PRs have the respective discussions, as well as issues on Leaky. We want to hear your feedback, and even more so we want to hear logical arguments if you feel like something is wrong. Although we examine many possible arguments before making changes, it's unfeasible to involve the entire community in every decision before it's developed.
There was a problem hiding this comment.
I can't seem to find a nondeprecated way of setting Blockdata values?
Also what am I supposed to do if I want the Players be able to use Itemids in my commands?
There was a problem hiding this comment.
Just use setMetaData()
There was a problem hiding this comment.
But I want no Metadata. I want to change the Blockdatavalue, the one responsible for woolcolors, woodtypes, ...
There was a problem hiding this comment.
@andre111 1f83111#commitcomment-4031009
But I want no Metadata. I want to change the Blockdatavalue, the one responsible for woolcolors, woodtypes, ...
This isn't the place for getting help writing plugins; irc would be much better suited for discussion about alternatives. However, you should look at BlockState. "I don't want to use that" -> There is no forward compatible contract that says wool color or other pieces of data are strictly part of what you think is a "data byte".
There was a problem hiding this comment.
Ok that helped
There was a problem hiding this comment.
Now many Materials carry other names than in-game, and the ids are deprecated :). Is the idea of custom blocks dead (thinking of the plugins API)?
There was a problem hiding this comment.
I posted this as a reply to the email thread, but github put it up by the line comments, so I'm reposting it back down here:
Can we start making a list somewhere of deficient APIs? That might help move things along in terms of bringing us back up to where we were. What would be the recommended format for something like that? A JIRA ticket?
There was a problem hiding this comment.
I've created a JIRA issue for tracking issues under this category https://bukkit.atlassian.net/browse/BUKKIT-4772 This should help us get started in moving these APIs forward.
There was a problem hiding this comment.
I have a bit of an issue with this, as I am looking for compatibility in the long-term. How am I supposed to get an id from the config now, and create an itemstack or get the material allocated to the id without deprecation? I would hope that Material.getMaterial(id) would never break...
There was a problem hiding this comment.
@JamesNorris: Item IDs won't be supported in future versions of Minecraft - So Bukkit can't use them. Dinnerbone tweeted on this several times, see for example https://twitter.com/Dinnerbone/status/383725505805307904
For now, you can use @SuppressWarnings("deprecation") and a noticeable warning message or auto-convert IDs to Material in config, I guess.
There was a problem hiding this comment.
It would be nice if there were an official statement from the Bukkit team on the forums (as a sticky in the Plugin Development section?) regarding the magic-id-deprecation strategy. I fully understand that it's necessary (and being driven by Mojang's upcominh changes in MC 1.7+), but the sudden appearance of a load of deprecated methods in Bukkit 1.6.4 is causing a lot of confusion and consternation among developers, particulary when the Javadocs make no mention of alternatives.
Good practice would be not to deprecate a method until there's a useful replacement. I will assume the Bukkit team have a good reason in this case for not following that practice, but some official explanation of what we developers should be doing would be appreciated. As far as I know, I should just continue using these deprecated methods for now and ignore the various warnings from my IDE; is that truly the case? And if I should just carry on using them, why are they deprecated at all yet?
There was a problem hiding this comment.
@desht They've been deprecated because they're trying to encourage developers to go on and switch to alternative methods where available in order to mitigate the issues that will arise if developers haven't switched over once the actual removal of IDs happens.
There was a problem hiding this comment.
@JamesNorris you (and every other plugin author) will need to stop supporting numeric block/item id's in your plugin's config files. Allow them for now, but have your plugin issue a warning (or maybe even auto-replace the numeric ID with a Bukkit material name) when they're detected. Material.getMaterial(String) or Material.matchMaterial(String) already work fine.
What's more problematic is that there is no way in Bukkit of parsing a block-material/block-data combination in a config file without having a numeric block-data value. E.g. how to specify a ladder attached on the north face? Right now, we can easily parse something like "ladder:2" (2 meaning north-facing for a ladder), but "ladder:north" needs extra coding (and to make this work for every block type needs lots of extra coding!). I think this is a candidate for some new Bukkit API, since many plugins will want this sort of functionality (e.g. a /give command, or any plugin which support templates for creating structures...). It would be a shame to have a load of duplicated code to do this.
There was a problem hiding this comment.
@gmcferrin "where available" - fine. That wasn't my point. There are many methods which have been deprecated where there is no non-deprecated alternative. That is what deserves some kind of official announcement, IMHO.
There was a problem hiding this comment.
@desht 1f83111#commitcomment-4277055
There are many methods which have been deprecated where there is no non-deprecated alternative. That is what deserves some kind of official announcement, IMHO.
The strategy is as it always has been; To have proper API for as much as possible. Deprecated methods clearly communicate what the Bukkit Team has identified as unreliable code. You can rest assured that they will not simply remove a method that does not have an alternative before ensuring a solution. Deprecating a method does not absolve the team of improper API support.
Encouraging developers to avoid unreliable code by seeking out existing alternatives and be prepared for when new alternatives are created is a good practice. Can you expand upon what you'd expect any announcement to clarify further?
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
Deprecation of code does not require a replacement. If the Bukkit Team created an announcement for every action they took that was outside of the statistically most common implementations we'd have to rename them the Bukkit Announcers. I'm still unclear as to why this action is so deserving of an announcement versus any other. The action itself indicates what should be done. An announcement would have no further clarifying information for the community to benefit from.
There are no gaps since nothing has been removed.
The length of this commit's comments is not sufficient reason to clarify a valid usage of deprecation. Many of these comments highlight developer education issues and not API problems. An announcement will not solve a lack of education. Developers would still not know what to do and they'd still generate the same commentary and requests. Continuing as normal with forum posts requesting help and Leaky tickets requesting API improvements is a reasonable path. This action in particular is designed to encourage those interactions. Not all developers read community announcements. All developers will encounter the deprecated tags.
There was a problem hiding this comment.
Also, removing an API to something that is actually removed in Vanilla is not a "gap" in the API, you know.
You may also consider that those deprecated methods will not be the only API that will be removed, it depends on what the 1.7 /actually/ changes. For now we're just sure that magic values will no longer make sense. But for example, the whole Material enum could be removed and replaced by something different.
There was a problem hiding this comment.
It's all well to deprecate things, having in mind upcoming changes. In my opinion at least the commit could have used a little bit more of a description, to inform a range of people swiftly at a very low cost.
Some context information is missing here, "magic value" may be something that items have in common, but it does not really describe what is going to happen and/or why, given the differing arguments and prospects for the individual cases.
An announcement directed at plugin developers could make sense for the case of a fundamental change, simply to put in the community of those, once it is known to be an actual change.
Trying to look for alternative ways to implement block-type lookup while virtually all current ways will probably not exist anymore some time in the near future, does not seem to be a very efficient thing to do - of course i can start thinking about potential future-designs and effectively shorten the time needed to adapt my own plugins.
There was a problem hiding this comment.
Since both generate and generateBlockSections are deprecated now, what is the way to generate terrains? Mod Minecraft? It would help if the comments included more than just "Magic value." Something akin to "Magic value, use Material.getMaterial(String) instead." or "Magic value, no API or workaround available."
There was a problem hiding this comment.
@lgoss007 Basically, we're waiting to see which way Minecraft goes WRT to block IDs. We know they'll be changing soon, but we don't know how or what they're going to be replaced with. Until we do, all this code is deprecated, because we know its probably at least going to be changing in some breaking way.
Are map ID's actually magic values as such? The ID is not an arbitrary value in this case; it's a sequence number which increments when a new map is created. In addition, it corresponds directly to the return value of ItemStack#getDurability(), a method which is not being deprecated.