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.

Sign up for GitHub

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

Jump to bottom

Make (most of) the examples executable again #695

Merged
merged 12 commits into from Jun 19, 2018
Merged

Make (most of) the examples executable again #695

merged 12 commits into from Jun 19, 2018

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented May 27, 2018 •
edited

As mentioned in #437 (comment) I went through all code examples and tried to make sure that they actually compile with API version 7.

I also make some changes to simplify future code checks.

I couldn't verify some parts such as whether the method signature is still correct if only a method is contained in that code example. There are currently 292 Java Code Blocks in there.

Fixes #571.

@ST-DDT
Make (most of) the examples executable again.
73cfbce 
Also make some changes to simplify future code checks.
@ST-DDT ST-DDT added needs review The submission is ready and needs to be reviewed outdated docs The API has changed and the docs are outdated labels May 27, 2018
@Spongy
Copy link

Spongy commented May 27, 2018 •
edited

A preview for this pull request is available at https://cdn.rawgit.com/Spongy/SpongeDocs-PRs/1cabbd2/.

Here are some links to the pages that were modified:

Since the preview frequently changes, please link to this comment, not to the direct url to the preview.

ST-DDT
ST-DDT commented May 27, 2018
View reviewed changes
source/plugin/scheduler.rst Outdated
| | | could look like "fooplugin-A-12". No two active tasks will have the same | |
| | | serial ID for the same synchronization type. If a task name is specified, | |
| | | it should be descriptive and aid users in debugging your plugin. | |
+-----------------+-------------------------+---------------------------------------------------------------------------+----------------------------------------------------+
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Revert this

ST-DDT
ST-DDT commented May 27, 2018
View reviewed changes
source/plugin/wgen/customwgen.rst Outdated
buffer.setBiome(x, y, BiomeTypes.FOREST);
} else if (x * x + y * y < BEACH_RADIUS) {
buffer.setBiome(x, y, BiomeTypes.BEACH);
for (int z = min.getY(); z <= max.getY(); z++) {
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Use Z

@ST-DDT
Copy link
Member Author

ST-DDT commented May 27, 2018

There are some "cosmetic" changes such as ... -> [...].
I did these for two reasons:

  • All code should look the same
  • I use these tokens to either remove them entirely or replace them with actual values automatically.
    (To simplify the code verification process - Regex FTW)

gabizou
gabizou requested changes May 29, 2018
View reviewed changes
source/contributing/implementation/datamanipulator.rst Outdated
@@ -214,7 +214,7 @@ There add a line to register (and create) your used keys.

.. code-block:: java

keyMap.put("health"), makeSingleKey(Double.class, MutableBoundedValue.class, of("Health")));
keyMap.put("health", makeSingleKey(Double.class, MutableBoundedValue.class, of("Health")));
Copy link
Member

Choose a reason for hiding this comment

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

All the key stuff needs to be updated to use the register method like so:

this.register(Key.builder().type(TypeTokens.BOUNDED_DOUBLE_VALUE_TOKEN).id("health").name("Health").query(of("Health")).build())

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

source/plugin/assets.rst Outdated
@@ -50,7 +50,10 @@ following code:
import java.nio.file.Files;

if (Files.notExists(configPath)) {
plugin.getAsset("default.conf").copyToFile(configPath);
Optional<Asset> asset = plugin.getAsset("default.conf");
if (asset.isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

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

Can simplify this to

plugin.getAsset("default.conf").ifPresent(asset -> asset.copyToDirectory(configPath));

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

source/plugin/entities/spawning.rst Outdated
@@ -35,8 +32,7 @@ For example, let's try to spawn a Creeper:
World world = spawnLocation.getExtent();
Entity creeper = world
.createEntity(EntityTypes.CREEPER, spawnLocation.getPosition());
SpawnCause spawnCause = SpawnCause.builder().type(SpawnTypes.PLUGIN).build();
world.spawnEntity(creeper, Cause.source(spawnCause).build());
world.spawnEntity(creeper);
Copy link
Member

Choose a reason for hiding this comment

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

Should be pushing to the cause stack frame for a SpawnType.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

source/plugin/items/creating.rst Outdated
}
Entity item = extent.createEntity(EntityTypes.ITEM, spawnLocation.getPosition());
item.offer(Keys.REPRESENTED_ITEM, superMegaAwesomeSword.createSnapshot());
extent.spawnEntity(item);
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here, need to add the SpawnType to the context.

Copy link
Member

Choose a reason for hiding this comment

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

Can wrap with:

try (CauseStackManager.StackFrame frame = Sponge.getCauseStackManager().pushStackFrame()) {
    frame.addContext(EventContextKeys.SPAWN_TYPE, SpawnTypes.PLACEMENT);
    extent.spawnEntity(item);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@Inscrutable Inscrutable added the high priority This needs to be fixed ASAP label May 30, 2018
@Inscrutable Inscrutable added this to the v7.X milestone May 30, 2018
@Inscrutable Inscrutable removed the needs review The submission is ready and needs to be reviewed label Jun 1, 2018
@ST-DDT
Copy link
Member Author

ST-DDT commented Jun 2, 2018

@gabizou I fixed the issues you pointed out in your comments. Is everything now correct?

gabizou
gabizou approved these changes Jun 17, 2018
View reviewed changes
Copy link
Member

@gabizou gabizou left a comment

Choose a reason for hiding this comment

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

minor things, but overall functional.

source/contributing/implementation/datamanipulator.rst Outdated

public SpongeHealthData() {
this(DataConstants.DEFAULT_HEALTH, DataConstants.DEFAULT_HEALTH);
this(DEFAULT_HEALTH, DEFAULT_HEALTH);
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have this not static imported, only because static imports are somewhat evil ambiguous as to where they (the constants) come from.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

source/plugin/entities/spawning.rst Outdated

Entity creeper = world.createEntity(EntityTypes.CREEPER, spawnLocation.getPosition());

try (StackFrame frame = Sponge.getCauseStackManager().pushCauseFrame()) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the cause stack manager is documented, but may want to add a code comment that the context needs to be wrapped in the frame because of the changes.

Likewise, mention that the plugin's PluginContainer is already pushed to the cause stack.

Copy link
Member Author

Choose a reason for hiding this comment

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

Like this?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that works.

@Inscrutable Inscrutable merged commit 9309dd0 into stable Jun 19, 2018
@ST-DDT ST-DDT deleted the fixes/code-compileable branch June 28, 2018 12:57
@ST-DDT ST-DDT mentioned this pull request Aug 26, 2018
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabizou gabizou gabizou approved these changes

@parlough parlough parlough approved these changes

Assignees
No one assigned
Labels
high priority This needs to be fixed ASAP outdated docs The API has changed and the docs are outdated
Projects
None yet
Milestone
v7.4.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ST-DDT @Spongy @gabizou @parlough @Inscrutable