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

Improve JavaDocs #92

Merged
merged 3 commits into from
Apr 28, 2018
Merged

Improve JavaDocs #92

merged 3 commits into from
Apr 28, 2018

Conversation

lucko
Copy link
Contributor

@lucko lucko commented Apr 12, 2018

I don't know if any of these changes are wanted (or welcome) or whatever - but I thought I'd PR them anyway. The only thing changed is formatting - there are no behavioural differences.

I've only just noticed that this repo is now under the SpongePowered umbrella. Does that mean it's now being actively maintained by Sponge - or did zml just want to hand over the responsibility?

If "improvement" PRs (as opposed to bug fixes) aren't going to be considered, then I'll stop submitting them & just maintain my own fork :) It would be nice to know the status - I know this probably isn't a priority for the Sponge team.

Introduce consistent formatting in Maven poms

The current poms have inconsistent formatting and indentation in some places. I rearranged & fixed indentation where it was appropriate to do so - without any change in build behaviour.

Use the correct "slashstar" style when generating license headers

The current "javadoc" style is reserved for JavaDocs - and is not meant to be used for license headers.

IntelliJ gives this suggestion/warning

👎

/**
 * Configurate
 * Copyright (C) zml and Configurate contributors
 *
 * and so on...
 */

👍

/*
 * Configurate
 * Copyright (C) zml and Configurate contributors
 *
 * and so on...
 */

JavaDoc (and other misc) improvements

These are all still WIP.

I feel like better documentation would go a long way in making configurate easier to use and understand.

The only breaking change is...

The method signature of TransformAction has been changed from

Object[] visitPath(SingleConfigurationTransformation.NodePath inputPath, ConfigurationNode valueAtPath)

to

Object[] visitPath(NodePath inputPath, ConfigurationNode valueAtPath)

SingleConfigurationTransformation is a package private class - it was previously impossible to implement the interface as a result.

The presently used "javadoc" style is reserved for JavaDocs - and is not meant to be used for license headers.
@parlough
Copy link
Contributor

We're actively maintaining the library, I just don't think anyone has it high on their priority list at the moment. So don't worry this and other improvements are still extremely welcome, and we'll try to get to them sooner rather than later.

So thanks and keep it coming :)

@lucko lucko force-pushed the javadoc branch 4 times, most recently from a57c2b8 to afe14ed Compare April 17, 2018 18:50
@lucko lucko changed the title [WIP] Improve JavaDocs Improve JavaDocs Apr 17, 2018
@lucko
Copy link
Contributor Author

lucko commented Apr 17, 2018

Ok - good to hear :)

I think this PR is ready now - would welcome any comments or further suggestions.

<groupId>org.checkerframework</groupId>
<artifactId>checker-qual</artifactId>
<version>2.4.0</version>
<optional>true</optional>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this - leave exclusions to consumers of this library.
google/guava#2721

Copy link
Contributor Author

@lucko lucko Apr 17, 2018

Choose a reason for hiding this comment

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

Would <scope>provided</scope> work?

Unlike the other dependencies, checker-qual isn't needed at runtime.

For dependencies like this it makes sense for consumers of the library to include it themselves if needed imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, you can try <scope>provided</scope>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Provided scope works - but means checker-qual has to be specified in each of the module poms.

It really seems like setting optional to true is the best solution here?

Indicates the dependency is optional for use of this library. While the version of the dependency will be taken into account for dependency calculation if the library is used elsewhere, it will not be passed on transitively

The dependency is optional for the use of configurate, and doesn't need to be passed on transitively.

import ninja.leaping.configurate.ConfigurationNode;
import ninja.leaping.configurate.SimpleConfigurationNode;
import ninja.leaping.configurate.loader.AtomicFiles;
import ninja.leaping.configurate.loader.ConfigurationLoader;
import ninja.leaping.configurate.util.MapFactories;
import org.junit.Ignore;
Copy link
Contributor

Choose a reason for hiding this comment

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

These should stay where they were.

@@ -22,6 +22,9 @@
import com.google.gson.stream.JsonReader;
import com.google.gson.stream.JsonToken;
import com.google.gson.stream.JsonWriter;

Copy link
Contributor

Choose a reason for hiding this comment

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

spacing is wrong - should be all together

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm missing something, the rest of the project seems to format things this way already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope nevermind, my bad. Will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok should be sorted now - I've changed my codestyle config and organised everything using the existing style

@lucko
Copy link
Contributor Author

lucko commented Apr 27, 2018

I've published a forked version of configurate containing these changes - so this can now all be previewed here:

https://javadoc.io/doc/me.lucko.configurate/configurate-core/3.4

There was quite a bit of reshuffling required to merge the current PRs together, since this one in particular has quite a large diff which spans the whole project. Those merge commits can all be found here, including a number of other changes to merge the other PRs in with the same style of javadoc improvements.

lucko/configurate@3.3...3.4 & https://github.com/lucko/configurate/releases/tag/3.4

Hopefully they can be of some use when there's activity again here.

@kashike
Copy link
Contributor

kashike commented Apr 27, 2018

@Meronat look good to you?

@parlough
Copy link
Contributor

@kashike Will review this and the other PRs when I return home tonight.

@kashike kashike self-assigned this Apr 28, 2018
@kashike kashike merged commit 676e8ba into SpongePowered:master Apr 28, 2018
@lucko lucko deleted the javadoc branch June 26, 2018 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants