Skip to content

Fix EOLs not being respected in modified files per #183#193

Merged
hakancelikdev merged 5 commits into
hakancelikdev:masterfrom
CAM-Gerlach:i183
Sep 9, 2021
Merged

Fix EOLs not being respected in modified files per #183#193
hakancelikdev merged 5 commits into
hakancelikdev:masterfrom
CAM-Gerlach:i183

Conversation

@CAM-Gerlach
Copy link
Copy Markdown
Contributor

@CAM-Gerlach CAM-Gerlach commented Sep 6, 2021

  • Fix unhandled ctypes-related MyPy errors on Windows causing committing to fail
  • Add regression tests for issue LF is converted to CRLF on windows #183
  • Fix LF is converted to CRLF on windows #183 : Respect current file EoLs instead of coercing to platform default
  • Add unit tests for new utils.read() functionality
  • Pre-commit all passes locally
  • Test suite all passes locally
  • Add changelog entry? [Awaiting maintainer guidance given lack of unreleased section and non-standards-conformant format]

Typically with Pytest, the idiomatic approach would of course be using fixtures and parametrize maximize code re-use and minimize duplication, but given the existing tests all appear to conform to legacy unittest conventions, I followed the latter rather than the former, thus the non-trivial amount of similar code. I also pulled the common persistent temporary file context manager into a separate utility module, as was done elsewhere, to avoid duplication between test modules. As I'm not very familiar with using unittest for serious projects, let me know if there's any convention/patterns I should be following but didn't.

Fix #183

@CAM-Gerlach
Copy link
Copy Markdown
Contributor Author

@hakancelik96 Please review, thanks.

@CAM-Gerlach
Copy link
Copy Markdown
Contributor Author

Regarding the check failure, starting multi-line docstrings on their own line is my standard approach and aligns with the Numpydoc format specification, but is not what existing codebase uses (it appears to roughly follow Sphinx convention, which does not mandate the summary line on its own line, and in fact has a number of more severe PEP 257 violations, e.g. summary line split across multiple physical lines, that would presumably get flagged), which is why I was careful to follow it.

I would suggest either conforming the codebase to follow the standards your CIs enforce (given this is trivially easy with regex, sed, tooling, etc), disabling this particular check, or at the very least making it very explicit in the contributing guide that contributors are expected to conform any lines they touch to a particular new standard. Otherwise, contributor confusion and frustration is likely to result, as their code will fail the checks despite taking pains to carefully conform to the existing convention, regardless of their own personal preferences. Especially for a small codebase like this with few contributors and no in-flight PRs, I'm not sure there's a compelling reason to allow the existing code to conform to different, lower standards than new contributors are expected to adhere to.

@hakancelikdev
Copy link
Copy Markdown
Owner

Thanks, everything looks correct.

Among the to-do list is to refactor the tests, I can't argue that the tests are very good, I will improve them.

@hakancelikdev hakancelikdev merged commit acaebf5 into hakancelikdev:master Sep 9, 2021
@CAM-Gerlach
Copy link
Copy Markdown
Contributor Author

Among the to-do list is to refactor the tests, I can't argue that the tests are very good, I will improve them.

Yeah; since you're already using Pytest as the runner, you can gradually implement more idiomatic code rather than having to port everything at once. I didn't do so with my new tests to be consistent with your existing style, but I can if you'd prefer. [Edit: Looks like that's a moot point now].

One note: I would suggest caution in requiring contributors to add their full name and email address to a public plain text file, since that can easily be scraped by scammers using generic tools (much easier than having to extract it from the Git metadata), given that Git and GitHub already authoritatively track this information. I figure mine is probably finable enough other places that it didn't make much difference, but I could see it causing privacy concerns for future contributors.

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.

LF is converted to CRLF on windows

2 participants