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

Add config flag and support .yaml #29 #81

Closed
wants to merge 6 commits into from

Conversation

Mctalian
Copy link

@Mctalian Mctalian commented Oct 6, 2022

Should handle #29 if you want to use .ls-lint.yaml instead. Also allows for specifying a config file ls-lint -config ./relative/path/to/config/.ls-lint.yaml.

You can specify the relative path to a new working directory with -pwd flag. This could be useful for multi-language monorepos where projects have different directory naming conventions. ls-lint -pwd ./relative/path -config ./relative/path/.ls-lint.yml

Hyperfine performance results (2022/10/07 1:30 PM MT)

build results
master Time (mean ± σ): 85.0 ms ± 1.8 ms [User: 48.4 ms, System: 32.4 ms]
this branch Time (mean ± σ): 84.6 ms ± 2.1 ms [User: 48.4 ms, System: 32.2 ms]

Run on:

Hardware Overview:

      Model Name: MacBook Pro
      Model Identifier: MacBookPro16,1
      Processor Name: 6-Core Intel Core i7
      Processor Speed: 2.6 GHz
      Number of Processors: 1
      Total Number of Cores: 6
      L2 Cache: 768 KB
      L3 Cache: 6 MB
      Hyper-Threading Technology: Enabled
      Memory: 16 GB

I didn't see any contribution guidelines, so let me know if you'd like me to do something else.

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2022

Codecov Report

Base: 66.89% // Head: 76.00% // Increases project coverage by +9.11% 🎉

Coverage data is based on head (926d008) compared to base (9ecddb6).
Patch coverage: 72.22% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   66.89%   76.00%   +9.11%     
==========================================
  Files          13       14       +1     
  Lines         441      471      +30     
==========================================
+ Hits          295      358      +63     
+ Misses        129       85      -44     
- Partials       17       28      +11     
Impacted Files Coverage Δ
main.go 55.26% <52.63%> (+55.26%) ⬆️
read_config_file.go 76.92% <76.92%> (ø)
linter.go 79.83% <100.00%> (+8.01%) ⬆️
rule_camelcase.go 92.30% <0.00%> (+23.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@loeffel-io
Copy link
Owner

thanks @Mctalian for the PR! To specify a config file should not change the root - maybe there should be another option for this? 🤔

@Mctalian
Copy link
Author

Mctalian commented Oct 7, 2022

thanks @Mctalian for the PR! To specify a config file should not change the root - maybe there should be another option for this? 🤔

Sure! I can look into changing that.

Any tips on how to get past the codecov checks? I can't test the main func so I'm a bit stuck 😅

@Mctalian
Copy link
Author

Mctalian commented Oct 7, 2022

@loeffel-io I added the -pwd flag which will change the root. -config no longer changes the root.

I moved the logging of errors out of main to try to increase coverage - it did increase, but not enough to pass the patch check. Not sure how much more I can increase that coverage 🙁

@Mctalian
Copy link
Author

@loeffel-io how does this look? Anything else you'd like me to change?

@Mctalian
Copy link
Author

Mctalian commented Nov 2, 2022

Bump @loeffel-io

@loeffel-io loeffel-io self-requested a review November 5, 2022 15:09
@groenroos
Copy link

Keen to see this merged and released, as being able to define a custom config file path would unblock us! 🙏

@Mctalian
Copy link
Author

Bump @loeffel-io

@Mctalian
Copy link
Author

Mctalian commented Dec 1, 2022

Hey @loeffel-io anything that I can do to get this merged in?

@Mctalian
Copy link
Author

Mctalian commented Feb 7, 2023

@loeffel-io bump

@loeffel-io
Copy link
Owner

the complexity of this pr in case of maintenance is to high for this two simple flags

@loeffel-io loeffel-io closed this Feb 8, 2023
@Mctalian
Copy link
Author

Mctalian commented Feb 8, 2023

Super disappointed with that decision. Best of luck on maintaining this library. 👋🏼

@loeffel-io
Copy link
Owner

With v2.x this is provided by the config and workdir option: https://github.com/loeffel-io/ls-lint/releases/tag/v2.0.0-beta.0

Feel free to try it out

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.

4 participants