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

Config all docs: temporary titles, sorted, faster #4522

Merged
merged 5 commits into from Oct 15, 2019

Conversation

@FroMage
Copy link
Member

FroMage commented Oct 11, 2019

Version with temporary titles util we get real ones with @aloubyansky's PR.

@FroMage FroMage mentioned this pull request Oct 11, 2019
0 of 6 tasks complete
Copy link
Member

machi1990 left a comment

Hey @FroMage thanks for yet another impressive work on configuration doc generation.

I have added some comments, let me know what you think.

For the js suggested modifications, I have some local modifications - cascading modifications and I could not find a way to easily suggest them here. I cannot push them either cause I do not have the necessary permission. Do you want me to create a PR in your fork? Let me know what you think.

Thanks.

if(search == lastSearch)
return;
var startTime = new Date();

This comment has been minimized.

Copy link
@machi1990

machi1990 Oct 12, 2019

Member

This seems to not be used anywhere.

This comment has been minimized.

Copy link
@FroMage

FroMage Oct 14, 2019

Author Member

fixed

return function(event){
var target = event.target;

var startTime = new Date();

This comment has been minimized.

Copy link
@machi1990

machi1990 Oct 12, 2019

Member

We can remove this variable too, it is not used anywhere.

This comment has been minimized.

Copy link
@FroMage

FroMage Oct 14, 2019

Author Member

fixed.

caption.children.item(0).appendChild(input);
input.addEventListener("keyup", initiateSearch);
input.addEventListener("input", initiateSearch);
inputs[input] = {"table": table};

This comment has been minimized.

Copy link
@machi1990

machi1990 Oct 12, 2019

Member

If the variable tables contains more than one element, we'll only track the last input element since we cannot use an object as keys for JS Object - accepts only strings as keys. I guess a WeakMap will have sufficed but I am not sure of its support for IE and older mobile browsers.

A workaround will be to generate an id attribute for the input and use this as inputs key. Consequently changing inputs[input] to inputs[input.getAttribute("id")].

This comment has been minimized.

Copy link
@FroMage

FroMage Oct 14, 2019

Author Member

Ah you're right, fixed.

final List<ConfigDocItem> configDocItems = entry.getValue();

sort(configDocItems);
ConfigDocSection header = new ConfigDocSection();

This comment has been minimized.

Copy link
@machi1990

machi1990 Oct 12, 2019

Member

I was thinking of extracting this part into a method findExtensionName(..) or something similar that can Version with temporary titles util we get real ones with https://github.com/quarkusio/quarkus/pull/4423.
We can probably add a few tests for the method too :-)

WDYT?

This comment has been minimized.

Copy link
@FroMage

FroMage Oct 14, 2019

Author Member

Well, it's temporary so I wouldn't bother.

@FroMage

This comment has been minimized.

Copy link
Member Author

FroMage commented Oct 14, 2019

Huh, not sure how to use the info added by #4423 since when I generate the all-docs I don't have these extensions in my classpath and I don't know their GAVs. I only know their lists of config roots, and some extensions have more than one.

@FroMage

This comment has been minimized.

Copy link
Member Author

FroMage commented Oct 14, 2019

When the annotation processor is invoked, I don't know the current artifact's GAV. I could guess and read the target/classes/META-INF/quarkus-extension.json file but if it's generated then I can't (and I'd rather want it to be).

I'm not sure how to proceed to get the extension name from the annotation processor. Perhaps @aloubyansky has an idea?

@aloubyansky

This comment has been minimized.

Copy link
Member

aloubyansky commented Oct 14, 2019

The JSON is generated in process-resources phase, which happens before the compilation phase. So you could be fine.
Otherwise, we'd need to somehow combine the JSON generation and what you are doing here to make sure all the info is available.

@machi1990

This comment has been minimized.

Copy link
Member

machi1990 commented Oct 14, 2019

Otherwise, we'd need to somehow combine the JSON generation and what you are doing here to make sure all the info is available.

+1 for this :-)

@FroMage

This comment has been minimized.

Copy link
Member Author

FroMage commented Oct 14, 2019

@aloubyansky except the APT processor is invoked in the deployment artifact and the extension descriptor is in the runtime one…

@aloubyansky

This comment has been minimized.

Copy link
Member

aloubyansky commented Oct 14, 2019

Given that the deployment artifact depends on the runtime artifact, at least the build sequence is going to be reliable. It's a matter of resolving the runtime artifact. I am not yet sure whether you have access to the Maven resolver api there.

@FroMage

This comment has been minimized.

Copy link
Member Author

FroMage commented Oct 14, 2019

Actually, we have some config roots in the runtime artifact and others in the deployment artifact.

@FroMage

This comment has been minimized.

Copy link
Member Author

FroMage commented Oct 14, 2019

I don't think we can use the Maven resolver during APT since we're in the process of compiling Java code, so artifacts can't be resolved yet at this point.

@aloubyansky

This comment has been minimized.

Copy link
Member

aloubyansky commented Oct 14, 2019

If you provide the workspace reader, they can be. Like the bootstrap's maven resolver does which you could theoretically re-use here. Actually you don't normally know what the corresponding runtime artifact is for a given deployment, do you? Unless you rely on the default naming convention.

@FroMage

This comment has been minimized.

Copy link
Member Author

FroMage commented Oct 14, 2019

Yeah, I don't.

Another option is to do the resolving when building the docs themselves. At this point all extensions have been packaged and deployed. But I don't know what they are. I only have the list of config root classes, at this point.

@aloubyansky

This comment has been minimized.

@FroMage

This comment has been minimized.

Copy link
Member Author

FroMage commented Oct 14, 2019

What does it do?

@aloubyansky

This comment has been minimized.

Copy link
Member

aloubyansky commented Oct 14, 2019

It generates the extensions.json for Quarkus repo.

@cescoffier cescoffier added this to the 0.25.0 milestone Oct 15, 2019
@cescoffier cescoffier merged commit 99c2b04 into quarkusio:master Oct 15, 2019
22 checks passed
22 checks passed
quarkusio.quarkus Build #20191014.11 succeeded
Details
quarkusio.quarkus (Build for Native Build JDK8 Linux) Build for Native Build JDK8 Linux succeeded
Details
quarkusio.quarkus (Maven Cache Linux Maven Repo) Maven Cache Linux Maven Repo succeeded
Details
quarkusio.quarkus (Maven Cache Windows Maven Repo) Maven Cache Windows Maven Repo succeeded
Details
quarkusio.quarkus (Native Tests amazon-dynamodb, amazon-lambda) Native Tests amazon-dynamodb, amazon-lambda succeeded
Details
quarkusio.quarkus (Native Tests artemis-core, artemis-jms, kafka) Native Tests artemis-core, artemis-jms, kafka succeeded
Details
quarkusio.quarkus (Native Tests elytron-security-oauth2, elytron-security, oidc) Native Tests elytron-security-oauth2, elytron-security, oidc succeeded
Details
quarkusio.quarkus (Native Tests flyway, hibernate-orm-panache, reactive-pg-client) Native Tests flyway, hibernate-orm-panache, reactive-pg-client succeeded
Details
quarkusio.quarkus (Native Tests hibernate-search-elasticsearch) Native Tests hibernate-search-elasticsearch succeeded
Details
quarkusio.quarkus (Native Tests infinispan-cache-jpa, infinispan-client, mongodb-client, mongodb-panache, neo4j, narayana-stm) Native Tests infinispan-cache-jpa, infinispan-client, mongodb-client, mongodb-panache, neo4j, narayana-stm succeeded
Details
quarkusio.quarkus (Native Tests jackson, jgit, kogito, kubernetes-client) Native Tests jackson, jgit, kogito, kubernetes-client succeeded
Details
quarkusio.quarkus (Native Tests jpa, jpa-postgresql, jpa-mysql) Native Tests jpa, jpa-postgresql, jpa-mysql succeeded
Details
quarkusio.quarkus (Native Tests jpa-h2, jpa-mariadb, jpa-mssql) Native Tests jpa-h2, jpa-mariadb, jpa-mssql succeeded
Details
quarkusio.quarkus (Native Tests main) Native Tests main succeeded
Details
quarkusio.quarkus (Native Tests resteasy-jackson, vertx, vertx-http, virtual-http) Native Tests resteasy-jackson, vertx, vertx-http, virtual-http succeeded
Details
quarkusio.quarkus (Native Tests spring-di, spring-web, spring-data-jpa) Native Tests spring-di, spring-web, spring-data-jpa succeeded
Details
quarkusio.quarkus (Native Tests tika, hibernate-validator, test-extension) Native Tests tika, hibernate-validator, test-extension succeeded
Details
quarkusio.quarkus (Run JVM Tests Build JDK8 Linux) Run JVM Tests Build JDK8 Linux succeeded
Details
quarkusio.quarkus (Run JVM Tests Linux JDK11 Build) Run JVM Tests Linux JDK11 Build succeeded
Details
quarkusio.quarkus (Run JVM Tests Linux JDK12 Build) Run JVM Tests Linux JDK12 Build succeeded
Details
quarkusio.quarkus (Run JVM Tests Run_TCKs) Run JVM Tests Run_TCKs succeeded
Details
quarkusio.quarkus (Run JVM Tests Windows JVM Build) Run JVM Tests Windows JVM Build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.