Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP: components-first-release #7
WIP: components-first-release #7
Changes from 33 commits
af8e810
7781d6f
657cd4b
15826a9
84ff0fa
aefd082
207a668
73ab5c5
5c550a6
9b58fd2
cccbe1a
7cd6fed
f8100cc
33477ad
8edc81c
ee3a393
e48e40a
99e1d8c
8baba22
cbd05c4
eb13794
c72bf5a
f5ff6b6
f541320
c6d97bf
dfc1798
935f48b
7dc6e58
2f3e319
ca98734
0c84712
1987956
efd5257
46df98a
4dc125b
bf83370
ff9a234
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
LGTM! The workflow name and triggers are well-defined.
The workflow name "Release" is clear and descriptive. The triggers are appropriate for a release workflow, allowing for both automated releases on version tags and manual triggering.
Consider adding a brief description comment at the beginning of the file to provide more context:
🧰 Tools
🪛 yamllint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Job setup looks good, but there are some indentation issues.
The job setup is appropriate for a Clojure release workflow. Using a specific environment is good for managing secrets.
There are some indentation inconsistencies. Please adjust the indentation as follows:
This will resolve the yamllint warnings and improve readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve clarity and fix the link in the added note.
The note about using mockingbird without ClojureScript is informative. However, consider the following improvements:
Here's a suggested revision:
📝 Committable suggestion
🧰 Tools
LanguageTool
Markdownlint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Approve the Overview section with minor suggestions for improvement.
The Overview section provides valuable information about Mockingbird's features and benefits. Consider these minor improvements for better readability and grammar:
These changes improve grammar and remove unnecessary wordiness.
📝 Committable suggestion
🧰 Tools
🪛 LanguageTool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Approve the added links with a minor formatting suggestion.
The addition of direct links to the Node.js and npm installation pages is helpful for users. To improve readability, consider adding a blank line before the list:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Approve the updated installation instructions with suggestions for improvement.
The updated installation instructions for both Clojars and npm are helpful. However, consider the following improvements:
package.json
with pre-configured dependencies.Here's a suggested improvement for the formatting:
We distribute our software via clojars (and intend to do so also by npm): deps.edn: + ``` clj moclojer/mockingbird {:mvn/version "0.0.1"}
You will also need to start a project with shadow-cljs and install the npm deps.
npm:
+
You will also need to start a project with shadow-cljs and install the npm deps.
npm:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve the expanded Usage section with suggestions for improvement.
The expanded Usage section with examples is very helpful for users. However, consider the following improvements:
Here's a suggested improvement:
you can pass arguments into our components simply as this:
As you can see, our components have specific built-in styles for you. You can see a handful tutorial on the parameters for styling the components here
you can pass arguments into our components simply as this:
As you can see, our components have specific built-in styles for you. You can see a handful tutorial on the parameters for styling the components here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Approve the updated Testing Locally section with suggestions for improvement.
The renamed section and added instructions for local testing are very helpful. However, consider the following improvements:
npm ci
does and how it differs fromnpm install
.Here's a suggested improvement:
+
npm ci
is used for clean installs, which is ideal for testing and continuous integration environments.npm ci
is used for clean installs, which is ideal for testing and continuous integration environments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Consider Centralizing the Version Number
The version number
"0.0.1"
is hardcoded in multiple files (build.clj
,package.json
, andpackage-lock.json
). To enhance maintainability and ensure consistency across the project, consider implementing a centralized version management system for future releases.🔗 Analysis chain
LGTM: Project metadata definitions are correct
The project metadata definitions look good for a first release. However, for future releases, consider implementing a more flexible version management system.
To ensure consistency across the project, let's verify if the version is used elsewhere:
To accurately verify if the version number
"0.0.1"
is used elsewhere in the codebase, please run the following script:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 127
Script:
Length of output: 470
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more recent version of the Closure Compiler.
Adding the Closure Compiler dependency is a great choice for optimizing the JavaScript output. However, the specified version
v20220803
is quite dated. Consider using a more recent version to ensure you have access to the latest optimizations and bug fixes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify an explicit version instead of "RELEASE" for 'slipset/deps-deploy'
Using
{:mvn/version "RELEASE"}
can lead to non-reproducible builds because it may resolve to different versions over time. Please specify an explicit version number to ensure build consistency and reliability.Apply this diff to set a specific version (replace
x.y.z
with the desired version)::deploy-clojars {:extra-deps {slipset/deps-deploy {:mvn/version "RELEASE"}} + {:mvn/version "x.y.z"}} :exec-fn deps-deploy.deps-deploy/deploy :exec-args {:installer :remote :sign-releases? false :pom-file "target/classes/META-INF/maven/com.github.moclojer/mockingbird/pom.xml" :artifact "target/mockingbird-0.0.1.jar"}}}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Remove trailing space
There is a trailing space at the end of line 3. While this doesn't affect the rendered output, it's good practice to remove trailing spaces for cleaner markdown.
Please remove the trailing space at the end of line 3.
🧰 Tools
Markdownlint