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

javadoc fixes for gestalt-module #156

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

javadoc fixes for gestalt-module #156

wants to merge 1 commit into from

Conversation

soloturn
Copy link
Contributor

No description provided.

@soloturn soloturn force-pushed the javadoc branch 3 times, most recently from 1d1a902 to 16dfb69 Compare November 18, 2024 06:47
Copy link
Contributor

@BenjaminAmos BenjaminAmos left a comment

Choose a reason for hiding this comment

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

At an initial glance, I have highlighted many spelling errors in the comments introduced here.

For getters, I would prefer that the description states that they return something, so "Returns a thing" is preferred over "Get a thing"/"Gets a thing".

Just because a constructor is a default constructor, that does not mean that you can describe it as "Default constructor". Even though the purpose is usually obvious, if you are aiming for complete JavaDoc coverage then we should at least make it quality documentation.

@soloturn soloturn force-pushed the javadoc branch 3 times, most recently from b157332 to 96d015d Compare December 25, 2024 12:20
Copy link
Contributor

@BenjaminAmos BenjaminAmos left a comment

Choose a reason for hiding this comment

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

I am hoping that this will be the last set of changes needed.

* @return Information on the contents on this module
*/
public ClassIndex getClassIndex() {
return classIndex;
}

/**
* @return A predicate that specifies whether a given class from the main classloader is a
* member of this module
* Gets a predicate which that specifies whether a given class from the main classloader is a
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Gets a predicate which that specifies whether a given class from the main classloader is a
* Gets a predicate that specifies whether a given class from the main classloader is a

@@ -84,6 +84,9 @@ public class ModuleEnvironment implements AutoCloseable, Iterable<Module> {
private final ModuleFileSource resources;

/**
* Creates a ModuleEnvironment with give bean context, modules and permissionProviderFactory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Creates a ModuleEnvironment with give bean context, modules and permissionProviderFactory.
* Creates a ModuleEnvironment with a given bean context, modules and permissionProviderFactory.

@@ -93,6 +96,9 @@ public ModuleEnvironment(BeanContext beanContext, Iterable<Module> modules, Perm
}

/**
* Creates a ModuleEnvironment with give bean context, modules, permissionProviderFactory, and classLoaderSupplier.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Creates a ModuleEnvironment with give bean context, modules, permissionProviderFactory, and classLoaderSupplier.
* Creates a ModuleEnvironment with a given bean context, modules, permissionProviderFactory, and classLoaderSupplier.

@@ -103,14 +109,19 @@ public ModuleEnvironment(BeanContext beanContext, Iterable<Module> modules, Perm
}

/**
* Creates a ModuleEnvironment with give bean context, modules, permissionProviderFactory,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Creates a ModuleEnvironment with give bean context, modules, permissionProviderFactory,
* Creates a ModuleEnvironment with a given bean context, modules, permissionProviderFactory,

@@ -103,14 +109,19 @@ public ModuleEnvironment(BeanContext beanContext, Iterable<Module> modules, Perm
}

/**
* Creates a ModuleEnvironment with give bean context, modules, permissionProviderFactory,
* classLoaderSupplier, and apiClassLoader.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* classLoaderSupplier, and apiClassLoader.
* classLoaderSupplier and apiClassLoader.

@Inject // TODO use another constructor.
public ModuleFactory() {
this(Thread.currentThread().getContextClassLoader());
}

/**
* Creates a Module out of a file Module.json in standard library path.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Creates a Module out of a file Module.json in standard library path.
* Creates a module using a module.json file from the standard library path.

@@ -90,6 +97,7 @@ public ModuleFactory(String defaultCodeSubpath, String defaultLibsSubpath) {
}

/**
* Constructor with classLoader, code, moodule subpath, metadata loaders.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Constructor with classLoader, code, moodule subpath, metadata loaders.
* Constructor with classLoader, code, module subpath, metadata loaders.

@@ -110,19 +122,27 @@ public static ModuleClassLoader create(Module module, ClassLoader parent, Permis
}

/**
* Get module id this classloarder belongs to.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Get module id this classloarder belongs to.
* Get module id this classloader belongs to.

public Name(String name) {
Preconditions.checkNotNull(name);
this.originalName = name;
this.normalisedName = name.toLowerCase(Locale.ENGLISH);
}

/**
* @return Whether this name is empty (equivalent to an empty string)
* Whether this name is empty (equivalent to an empty string)
* @return true if emppty
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @return true if emppty
* @return true if empty

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.

2 participants