Code fixes

[Kevin-Morales]

Fixes linter warnings, adjusted a couple of use statements, and made sure the tests still pass without modification.

[NSoiffer]

@Kevin-Morales: thanks for running the linter. I run clippy, but I didn't run a linter.

There are two things (maybe more) that I know I don't agree with the linter (or clippy) about. One is the "needless return" style. I find the code more readable when the returns are explicit. It is probably just my background where many of the languages I used in the past did that, so I look for them now. I'll admit I not 100% consistent with adding returns, but I would be happy to mostly have the linter tell me when there are missing.

The other style is that I prefer the line length to be 120 chars, not 80 chars. I'm old enough to know where the 80 char line length came from, but it no longer makes sense. The shorter lines means more lines are needed for the same code, so less that I see on a screen at once.

There are some individual linting "fixes" that I find bad style. For example, I just found out that the NVDA python style requires a comma after the final argument in a function in a situation like:

myFunctionCall(
    some long argument that is on its own line,
)

I find that very ugly. But it is their style, not mine, so I need to follow that.

Can you redo your changes/undo your changes with those two styles in mind. Thanks.

[NSoiffer]

I think one of your merges has a problem with a recent update to a test case. One of the fixes I made caused the test to recognize the Chemistry and I updated the expected result but I think you missed that. The correct result for "pointless_nones_in_mmultiscripts" in src/canonicalize.rs is:

 <math>
  <mmultiscripts data-chem-formula='6'>
    <mtext data-chem-element='1'>C</mtext>
    <mprescripts></mprescripts>
    <mn>6</mn>
    <mn>14</mn>
  </mmultiscripts>
 </math>

Can you submit the fix and I'll trigger the build again. Otherwise, the fixes look good.

Is there way to bring the linter into the VSCode environment so problems show while editing? Have you played with rustfmt? See #273 for a little discussion on that. It might something to use.

[NSoiffer]

FYI: the problem is from your commit 3389eb0 that reverted a commit I made. I don't know why you reverted my commit.

[Kevin-Morales]

Hi Neil, I have updated the test cases, as pointed out in your last message. After the fix, all of the tests that are not ignored by the test runner should pass now. Still working on the Python scripts and their code style. Best Regards, Kevin

…

On Tue, Oct 15, 2024 at 8:56 PM NSoiffer ***@***.***> wrote: I think one of your merges has a problem with a recent update to a test case. One of the fixes I made caused the test to recognize the Chemistry and I updated the expected result but I think you missed that. The correct result for "pointless_nones_in_mmultiscripts" in src/canonicalize.rs is: <math> <mmultiscripts data-chem-formula='6'> <mtext data-chem-element='1'>C</mtext> <mprescripts></mprescripts> <mn>6</mn> <mn>14</mn> </mmultiscripts> </math> Can you submit the fix and I'll trigger the build again. Otherwise, the fixes look good. Is there way to bring the linter into the VSCode environment so problems show while editing? Have you played with rustfmt? See #273 <#273> for a little discussion on that. It might something to use. — Reply to this email directly, view it on GitHub <#291 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVUXHK3U4U5MODFQGG25V3Z3W2TBAVCNFSM6AAAAABP35TH22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJVGQZTCOJTHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>