[SHRINKRES-338] Simplify/Improve Readme #204

xjusko · 2024-05-30T07:06:26Z

https://issues.redhat.com/browse/SHRINKRES-338

petrberan · 2024-06-06T13:03:51Z

README.md

+      <scope>import</scope>
+      <type>pom</type>
+    </dependency>
+    <!-- Other dependencies -->


Please remove

petrberan · 2024-06-06T13:09:08Z

README.md

+
+Maven coordinates follow `groupId:artifactId:[packagingType:[classifier]]:version` format. You can use these coordinates to resolve a file, where `G` is the group ID, `A` is the artifact ID, `P` is the packaging type, `C` is the classifier, and `V` is the version.
+
+#### Resolve a file using Maven coordinates:


The singular file implicates that only a single file will be resolved, however, all transitive dependencies will be downloaded as well. This should be something like "Resolved file with transitive dependencies using Maven coordinates" or similar

petrberan · 2024-06-06T13:12:56Z

README.md

+#### Returning Resolved Artifacts
+
+ShrinkWrap Resolvers provides various options to retrieve resolved artifacts. You can obtain them as files, streams, URLs, or specific archive types like JavaArchive, WebArchive, or EnterpriseArchive. For example:
+- Get as a URL: `Maven.resolver().resolve("G:A:V").withTransitivity().as(URL.class)`


I'd replace this with a code block, showing couple of usages, each - perhaps - with a comment explaining the return type ("Get as a URL", "Get as a Java archive", ...)

I'd use URL, JAR, WAR

README.md

petrberan · 2024-06-06T13:24:20Z

README.md

+File file = Maven.resolver().resolve("G:A:V").using(new RejectDependenciesStrategy(false, "G:B")).asFile();
+```
+
+#### ShrinkWrap Resolver Strategies


This is on the same level as examples above, yet should be nested in the list. Can this be reformatted better?

petrberan · 2024-06-06T13:24:37Z

README.md

+
+#### Available Strategies:
+
+- **AcceptAllStrategy (Equals TransitiveStrategy):**


Equals -> same as

petrberan · 2024-06-06T13:24:47Z

README.md

+- **RejectDependenciesStrategy:**
+  Rejects dependencies defined by G:A. By default, it is transitive, meaning all dependencies originating at G:A are removed. To change this behavior to reject defined dependencies but keep their descendants, use `RejectDependenciesStrategy(false, "G:A")`.
+
+- **TransitiveStrategy (Equals AcceptAllStrategy):**


Equals -> Same as

README.md

petrberan · 2024-06-06T13:31:27Z

README.md

+
+<sub>**Warning:** ShrinkWrap Resolvers doesn't consume settings.xml specified on the command line or in the IDE. It reads settings.xml files at standard locations: `~/.m2/settings.xml` and `$M2_HOME/conf/settings.xml`, unless overridden in the API or via System property.</sub>
+
+#### Define Maven repositories manually


The bit about overriding a repository with the same id should probably stay. Perhaps as a note as above?

petrberan · 2024-06-06T13:49:19Z

README.md

+
+In Maven projects, you likely specify dependencies in your pom.xml file. ShrinkWrap Resolvers allows you to avoid redundancy by automatically loading metadata from there.
+
+- To load dependencies from the POM:


These should be probably formatted as examples above

petrberan · 2024-06-06T13:51:08Z

README.md

+
+### Resolution of Artifacts from POM Files
+
+In Maven projects, you likely specify dependencies in your pom.xml file. ShrinkWrap Resolvers allows you to avoid redundancy by automatically loading metadata from there.


These two sentences seem like they've been extracted from two different paragraphs. I don't really see a connection there. Perhaps instead of redundancy we should focus more on convenience?

Or how do you understand them? @xjusko

petrberan · 2024-06-06T13:55:21Z

README.md

+
+ShrinkWrap Resolvers allows you to override any programmatic configuration via System properties.
+
+- `org.apache.maven.user-settings`


These should probably be explained a little bit. I'm not talking about full table (although it is an option), but providing just the properties and not what they stand for is confusing

petrberan · 2024-06-07T08:48:55Z

README.md

+
+## Embedded Maven
+
+Embedded Maven allows direct invocation of Maven builds from Java code, offering functionalities such as downloading and using desired Maven binaries, an uncluttered API for simple or complex builds, additional methods for build control (e.g., ignoring failures), and easy access to build outputs and artifacts. It can be invoked using ShrinkWrap Resolver API or custom Invoker and InvocationRequest instances.


Personally I would remove the "It can be invoked using ShrinkWrap Resolver API or custom Invoker and InvocationRequest instances." as it brings nothing to the overall readme. It even actually contradicts the old readme as Invoker and InvocationRequests are different from Embedded Maven

petrberan · 2024-06-07T08:58:13Z

README.md

+
+### Downloading Maven Binaries
+
+Embedded Maven facilitates downloading and using specific Maven versions from Apache web pages, caching downloaded binaries, and setting up Maven home for the build.


Maybe this is just me, but the word "facilitates" screams "I used Chat GPT to write this sentence" for me :)

petrberan · 2024-06-07T09:00:10Z

README.md

+Embedded Maven facilitates downloading and using specific Maven versions from Apache web pages, caching downloaded binaries, and setting up Maven home for the build.
+
+```java
+EmbeddedMaven.forProject("path/to/pom.xml").useMaven3Version(String version)


I would either used comments or explained this code block a little bit. For example, I'm missing the information about using cached Maven

petrberan · 2024-06-07T09:08:52Z

README.md

+Embedded Maven provides features like skipping tests, ignoring failures, logging Maven build invocation commands, and accessing built projects and their artifacts.
+
+```java
+EmbeddedMaven.forProject("path/to/pom.xml").setDebugLoggerLevel()


Same as above, these could be probably explained a little bit more. The skipTests and ignoreFailure is not shown at all

petrberan · 2024-06-07T09:09:23Z

README.md

+
+```java
+EmbeddedMaven.forProject("path/to/pom.xml").setDebugLoggerLevel()
+EmbeddedMaven.forProject("path/to/pom.xml").setLogger(yourInvokerLogger)


Missing variable type on the yourInvokerLogger variable. Should be InvokerLogger

petrberan · 2024-06-07T11:04:20Z

README.md

+EmbeddedMaven.forProject("path/to/pom.xml").setGoals("package").build().getDefaultBuiltArchive();
+```
+
+Daemon Build


This should be formatted as a headline I believe

petrberan · 2024-06-07T11:05:33Z

README.md

+EmbeddedMaven.forProject("path/to/pom.xml").setGoals("spring-boot:run").useAsDaemon().build();
+```
+
+### Experimental Features


I would also add the BuiltProject somewhere, just a line or two about what it is and the "extensive" example from previous Readme

README.md

xjusko · 2024-06-19T06:09:06Z

@petrberan requested changes should be fixed, can you review it again, please?

petrberan · 2024-06-21T13:08:34Z

README.md

+EmbeddedMaven.forProject("path/to/pom.xml").useInstallation(File mavenHome);
+// Uses local Maven installation available on your $PATH
+EmbeddedMaven.forProject("path/to/pom.xml").useLocalInstallation();
+// Uses the default Maven version, which is currently 3.3.9


I'm not so sure about that...

resolver/maven/api-maven-embedded/src/main/java/org/jboss/shrinkwrap/resolver/api/maven/embedded/DistributionStage.java

Line 30 in 7405607

String DEFAULT_MAVEN_VERSION = "3.9.6";

Probably best to remove the hardcoded version and link the code instead

petrberan

LGTM, thx @xjusko

petrberan · 2024-06-21T13:28:20Z

CI seems to be stuck, closing and opening the PR again