-
Notifications
You must be signed in to change notification settings - Fork 1
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
Added Super /Mega Linter #1
Conversation
.github/workflows/Linting.yaml
Outdated
|
||
# Remove this section when Super Linter is updated (https://github.com/dysonltd/tmag5273/issues/3) | ||
Clippy: | ||
name: Clippy |
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.
With a little jiggery-pokery it's possible to run clippy on doc examples too
Info can be found here - the bits you need are kind of dotted about but if you think it's worth implementing I've got a working example I was using on one of our other repos
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.
I think this would be good to add. Especially since the Super Linter isnt upgraded yet for the latest rust version (#3). Are you able to share here and il add it in?
ReadMe.md
Outdated
optimize throughput and accuracy. A dedicated | ||
INT pin can act as a system interrupt during | ||
low power wake-up and sleep mode, and can also be used by a | ||
microcontroller to trigger a new sensor conversion.An integrated |
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.
Missing space after full stop
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.
So Super Linter has a natural language linter, I wonder if this will flag these?
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.
Yup Textlint
https://textlint.github.io/
ReadMe.md
Outdated
integrates three independent Hall-effect sensors in the X, Y, and Z axes. A precision analog signal-chain along with an integrated 12-bit ADC digitizes | ||
integrates three independent Hall-effect sensors in | ||
the X, Y, and Z axes. A precision analog signal-chain | ||
along with an integrated 12-bit ADC digitizes |
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.
I appreciate that (presumably) the linter is keeping line lengths reasonably short, but it does make editing a bit of a pain due to having to change where all the newline characters are located.
I'm keen on the one-line-per-sentence approach as markdown renderers will automatically reformat into paragraphs, and this way you don't need to worry about where the line breaks occur. Then in the editor, line wrapping can be used to avoid sentences running off the right of the screen.
Is there a way we could get the linter to do this instead?
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.
Il have a look
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.
I like the idea of the convention being newline on every sentence, im happy to change that. Im currently experimenting with the markdownlint-cli --fix
, this looks promising.
ReadMe.md
Outdated
provides full 360° angular position information for both | ||
on-axis and off-axis angle measurement topologies. | ||
The angle calculation is performed using two | ||
user-selected magnetic axes. The device features | ||
magnetic gain and offset correction to mitigate the | ||
impact of system mechanical error sources.The TMAG5273 is offered in four different factory-programmed I2C addresses. The device also supports | ||
additional I2C addresses through the modification | ||
impact of system mechanical error sources.The TMAG5273 is |
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.
Missing space after full stop
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.
hmmm this is interesting, the markdownlint should of picked up on that
the library should be able to run in `no-std` environment such as baremetal. A | ||
[esp32c3](https://github.com/esp-rs/esp-rust-board)example is shown [here](./esp32-c3/src/main.rs). | ||
Due to it using a baremetal environment its recommended to go to that directory | ||
and call `cargo run` from within it and flash the code on to the device. |
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.
There's a lot of "it"s in this sentence, maybe worth rephrasing as a set of instructions?
After some googling and issues with getting the |
🦙 MegaLinter status: ❌ ERROR
See detailed report in MegaLinter reports |
.cspell.json
Outdated
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.
How does this spell check work? I tend to find them to just add noise where code uses nonstandard words and contractions all over the place.
.github/workflows/Linting.yaml
Outdated
name: Linting | ||
|
||
on: # yamllint disable-line rule:truthy | ||
# push: null |
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.
Is this line required still?
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.
I think we will be moving away from this actually after finding MegaLinter
https://megalinter.io/v8/
It handles the analysis slightly differently.
.jscpd.json
Outdated
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.
What does this file do?
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.
I believe this one is for the Natural language Check, but will need to check it.
.markdownlint.yaml
Outdated
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.
Is it possible to inherit a lint standard for markdown instead of having a long file of custom configuration like this?
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.
So you can run markdownlint with the standard and we would need to apply two settings around line length. This is how it was originally and was in json, but then SuperLinter needed a yaml file as it wasnt reading the json for some reason....
We should be able to solve this by moving to MegaLinter
.textlintrc.json
Outdated
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.
Like the spelling config, I'd prefer to limit the number of custom lint config files we need to track across repos as they add visual noise.
@@ -2,14 +2,21 @@ | |||
|
|||
## Summary | |||
|
|||
This internal crate is a utilities package designed for connecting the Embedded Targets to the Library as such it is not to be used as part of the library. | |||
This internal crate is a utilities package designed for connecting the Embedded |
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.
Again I'm not really a fan of line breaks in the middle of sentences because it makes editing a pain. We can have a discussion about this when all three of us are about.
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.
Agreed, i'm not sure how best to handle this. Because we cant break on a sentence end because some sentences go really long.
v1.0.0 of the cross platform i2c driver
Closing PR to reopen. To continue testing workflow |
Adding Linting to the Project
This PR is focused on adding the following languages checks
How it does this is up to discussion, currently investigating https://github.com/super-linter/super-linter/tree/v7.2.0.
Linting is currently set to be triggered on pull requests
Add Some form of Text Linting (TextLint)
Ensure All Markdown Files are being scanned
Get Linter Auto PR Fixes operational
Transition to using MegaLinter