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

Fix/additional coding style #279

Merged
merged 3 commits into from
Jun 26, 2023
Merged

Fix/additional coding style #279

merged 3 commits into from
Jun 26, 2023

Conversation

niyarin
Copy link
Contributor

@niyarin niyarin commented Jun 19, 2023

This PR adds clj-kondo's default off linter to clj-lint-action and fixes the warned code.
#273

@niyarin niyarin requested review from alumi and a team as code owners June 19, 2023 09:37
@niyarin niyarin requested review from r6eve and removed request for a team June 19, 2023 09:37
@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Merging #279 (63fda34) into master (cb87a24) will not change coverage.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##           master     #279   +/-   ##
=======================================
  Coverage   88.85%   88.85%           
=======================================
  Files          78       78           
  Lines        6512     6512           
  Branches      458      458           
=======================================
  Hits         5786     5786           
  Misses        268      268           
  Partials      458      458           
Impacted Files Coverage Δ
src/cljam/tools/cli.clj 59.74% <0.00%> (ø)
src/cljam/io/fasta/reader.clj 91.71% <100.00%> (ø)

Copy link
Contributor

@r6eve r6eve left a comment

Choose a reason for hiding this comment

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

This PR looks good to me.
In addition, according to Clojure Style Guide, I'd prefer the warn of Shadowed var. Cljam uses clojure.core vars as the name of fileds, and it's not easy to rename all fields that are used. So, I recommend to use :refer-clojure :exclude [VAR], and clojure.core/VAR if VAR are used as both user-defined vars and core vars in the same file.

Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the coding conventions. Your efforts are greatly appreciated 👍
I think the changes look good, except for the point @r6eve made.
Further modifications to :shadowed-var and :missing-docstring would be nice. Commits for these may be included in this pull request, or they may be established separately.

@niyarin
Copy link
Contributor Author

niyarin commented Jun 26, 2023

I will fix:shadowed-var and :missing-docstring in another PR.

@alumi alumi requested a review from r6eve June 26, 2023 02:31
Copy link
Contributor

@r6eve r6eve left a comment

Choose a reason for hiding this comment

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

I got it. Thank you for this and another PR.

@r6eve r6eve merged commit 09f81da into master Jun 26, 2023
@r6eve r6eve deleted the fix/additional-coding-style branch June 26, 2023 09:15
@niyarin niyarin mentioned this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants