search

nzhagen / jcamp

A set of Python utilities for reading JCAMP-DX files.
MIT License
52 stars 31 forks source link

Possible conventional changes? #27

Open sumpfralle opened 1 year ago

sumpfralle commented 1 year ago

Thank you for providing jcamp!

After taking a look at the repository I would recommend a few changes (and I offer to submit the relevant changes for some of these). Of course, these are just suggestion, which may not necessarily be in line with the style, which you envision.

I am willing to provide proposals for all of the above (except for the git tags - these cannot be created via a pull request).

What do you think? Would any of the points above be suitable for jcamp?

nzhagen commented 1 year ago

I follow many but not all of the PEP8 recommendations -- the 80-char line recommendation especially, since I generally use length 120 lines, and occasionally longer when it suits the code. However, I definitely agree about the naming inconsistency (when did I put in those stupid uppercase names?), and I have modified the current function names to be more consistent. I will take a look into the git tags -- I haven't spent time with those before.

If you have a recommendation about what you would change in the testing suite, I'd be glad to take a look at them. I don't have a strong preference for the current approach, and so if there is something better -- without introducing much extra complexity -- I'd be very glad to make changes.

Rerunning the testing suite just now, I can see that there is a bug in the "DUP" code parsing. I don't have time to debug that right now, so I'll open an issue on it and get back to it when I can.

I ran the code style recommender "flake8" and accepted some of its recommendations. Some of my own coding style conflicts with its default (such as my use of # for debugging and ## for comments), but it did help me locate some things that should have been done better. That's implemented now.

sumpfralle commented 1 year ago

Thanks for your quick response and for taking a look at the details!

Regarding PEP8: I think, the whitespace issues (especially missing whitespace after comma) are the ones, which hurt my eyes the most :)

Regarding style in general: I really enjoy the Python ecosystem since there really is just one style (with very little variety). Thus I gladly threw away all my personal preferences quite some time ago in order to just not feel offended by code from other people. In the world of C or elsewhere, I suffer a lot when seeing someone else's code - that is a much worse situation. Thus I would kindly invite you to revisit the concept of black - maybe it provides you with peace of mind (as it did for me) :)

(I will stop preaching about style now ...)

Regarding the tests (travis/github actions): right now the test suite is just not executed automatically at all, or? Maybe you want to take a look the github actions for running the unittests (the unittests are fine, from my point of view)? Or should I prepare something?

nzhagen commented 1 year ago

Actually, the current unit tests were provided not by me but by another contributor. And when I run the tests, I noticed that they were catching a parsing error. I didn't spend the time to look into that for a while, but I found the error now -- a misinterpretation of the "DUP" code spec. Now we should be able to run the tests automatically and rely on the result. One annoyance that remains is that the unit test complains about a function missing an argument where it actually has that argument everywhere, as far as I can tell. In any case, I'll look into GitHub actions.

nzhagen commented 1 year ago

I took a look into GitHub actions and see that they've made a lot of new tools in the past few years, while I've been deeply involved elsewhere. If you have some suggestions about the implementation, I'd be very glad to get them into the repo!