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

Change link numbers to short names #8

Closed
wants to merge 2 commits into from

Conversation

ferdnyc
Copy link

@ferdnyc ferdnyc commented Dec 8, 2023

A suggestion for your (very old, but meh) logging PR. I saw you were renumbering the links in the README — IMHO, using numbers is just a way to make yourself crazy. This replaces all of the numbered links with short names. (The longest is wiki_example, many others are only 2-5 characters. Still way more evocative and maintainable than numbers.)

Copy link

This pull request has conflicts, please resolve those so that the changes can be evaluated.

@lkk7 lkk7 force-pushed the feature/logging branch 2 times, most recently from db95e2e to ff24cde Compare April 14, 2024 16:30
* Implement basic library logging

pydot currently prints some error messages to standard output
(`stdout`). Printing anything is considered bad practice for libraries
[1] [2]. This becomes even more disturbing if the prints contain long
Graphviz output or DOT strings.

[1]: https://stackoverflow.com/questions/4201856/error-handling-strategies-in-a-shared-library-c
[2]: https://www.reddit.com/r/C_Programming/comments/97ebfj/redirect_stdout_and_stderr_to_a_buffer/e47n9kh/

This commit lays the groundwork for switching from printing to logging
by providing a basic library logging setup and some user documentation
on how to read the logs. Later commits will replace the actual printing
by logging.

Discussed in pydot#171 and pydot#231.

* Fixup: Black reformatting

* Fixup: Use `__name__` to name loggers

* Fixup: Add parent logger, Reorder registration

Notes:
- I am aware that in `__init__.py`, the placement of the logger
  registration before the imports of the submodules leads to additional
  warnings in code checkers:
  From pylint:
      C0413: Import "from pydot.exceptions import *" should be placed at the top of the module (wrong-import-position)
      C0413: Import "from pydot.core import *" should be placed at the top of the module (wrong-import-position)
  From flake8:
      E402 module level import not at top of file
      E402 module level import not at top of file
  I do not really see how we can avoid this if we want to log a first
  message early during pydot initialization. Also note that the
  concerned imports are all pydot "internal" submodules and that for
  `__init__.py` importing those is its main task, so this having a
  central place in the file does not seem unnatural.
- In `test/pydot_unittest.py`, added a final `importlib.reload(pydot)`
  to prevent subsequent tests from failing, probably caused by one of
  the caveats mentioned in:
  https://docs.python.org/3/library/importlib.html#importlib.reload

* Fixup: Documentation reordering 1

* Fixup: Documentation reordering 2

* Fixup: Documentation changes (minor)

Notes:
- Removed the hint that the parent logger can be used to control all
  pydot loggers, because this is basic knowledge covered by the Python
  documentation to which we already refer.

* Renumber links, Update PyPI URL in README.md

* Fixup: update the changelog

* Fixup: stabilize black usage

---------

Co-authored-by: lkk7 <[email protected]>
Copy link

All conflicts have been resolved, thanks!

Copy link

This pull request has conflicts, please resolve those so that the changes can be evaluated.

@ferdnyc
Copy link
Author

ferdnyc commented Apr 30, 2024

Closing as I've retargeted it to the main repo.

@ferdnyc ferdnyc closed this Apr 30, 2024
@ferdnyc ferdnyc deleted the readme-links branch May 3, 2024 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants