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

Codacy warning fixes #65

Merged
merged 2 commits into from Jan 4, 2024
Merged

Codacy warning fixes #65

merged 2 commits into from Jan 4, 2024

Conversation

ghost
Copy link

@ghost ghost commented Jan 4, 2024

  • Fixes line width in python documentation
  • Markdown bullet point spacing
  • Default nonTOF_distance_threshold in consistency test to 0.f

Fixes line width in python documentation, markdown bullet point spacing, and default nonTOF_distance_threshold in consistency test to 0.f
@ghost
Copy link
Author

ghost commented Jan 4, 2024

@KrisThielemans, can you take a look and confirm these changes?

@KrisThielemans
Copy link
Collaborator

thanks! Looks good to me. Tests are running, but will work of course.

sadly, Codacy isn't running on this repo. Maybe you can run it manually through the markdown checker?

@ghost
Copy link
Author

ghost commented Jan 4, 2024

Maybe you can run it manually through the markdown checker?

I am not 100% sure what markdown checker you are referring too but Prettier should handle it. It also matched the results of this one https://webutility.io/markdown-beautifier.

@KrisThielemans
Copy link
Collaborator

great. thanks. Codacy recommended another one, but I'm sure this is fine.

@KrisThielemans KrisThielemans merged commit 53dbdc1 into NikEfth:tof_sino_UCL Jan 4, 2024
@ghost
Copy link
Author

ghost commented Jan 4, 2024

This PR did not seem to resolve the issue. https://app.codacy.com/gh/UCL/STIR/pullRequest?prid=12651966

@ghost
Copy link
Author

ghost commented Jan 4, 2024

I don't understand why (1.)

- Test non-TOF ROOT and STIR consistency, particularly the rotation.

(line 12) fails because of "Spacing between a list item’s bullet and its content violates a given style" while (2.)

- `README.md`: This file.

(line 25) is okay... The only difference is the second example starts with a grave accent/ back tick, and maybe it is picking up on that? Previous is incorrect, because (3.)

- `SourceFiles/`: Contains 8 `generate_image` parameter files and GATE macro files for the emission source positions. One pair of files for each simulation.

fails for the same reason.

@KrisThielemans
Copy link
Collaborator

Ah well. Codacy is apprently using https://github.com/remarkjs/remark-lint

I see the following full message on Codacy

Reported by remark-lint
Time to fix: 5 minutes
These files are included to :

  • Test non-TOF ROOT and STIR consistency, particularly the rotation.
  • Test the TOF STIR implementation is correct.
    Why is this an issue?
    Warn when the spacing between a list item’s bullet and its content violates a given style.

Options: 'tab-size', 'mixed', or 'space', default: 'tab-size'.

Fix
remark-stringify uses 'tab-size' (named 'tab' there) by default to ensure Markdown is seen the same way across vendors. This can be configured with the listItemIndent option. This rule’s 'space' option is named '1' there.

See Using remark to fix your Markdown on how to automatically fix warnings for this rule.

So, I guess we could try and run remark offline, or configure it appropriately, or... ignore the rule, as it displays just fine. I'm ok with the latter option, as there's more important things in life, but let me know what you want to do.

@ghost
Copy link
Author

ghost commented Jan 5, 2024

IMO, it is being too strict on a trivial issue for a non pre-commit enforcing change. If this can be ignored, let's just. do that.

@KrisThielemans
Copy link
Collaborator

Looks like Codacy still isn't happy about the constructor either. Oh well. Not sure why. I'll ignore.

@ghost
Copy link
Author

ghost commented Jan 5, 2024

That one makes no sense to me because it's value is set with this PR and, if it wasn't, codacy should warn about some others too. I'm sorry, I don't understand.

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.

1 participant