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.

Sign up for GitHub

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

Jump to bottom

Restructure package and module organization #230

Closed
peternowee opened this issue May 11, 2020 · 3 comments
Closed

Restructure package and module organization #230

peternowee opened this issue May 11, 2020 · 3 comments
Labels
api discussion internals
Milestone
2.0.0 (or later)

Comments

@peternowee
Copy link
Member

This issue is split off from a discussion in #171 (and #218 and #219) in which @ankostis and I were discussing error handling and logging. The question was raised if pydot.py and dot_parser.py should share a common custom exceptions hierarchy and a common parent logger. This in turn raised questions about the current package structure.

@peternowee wrote on 2020-05-01:

I run into even bigger questions, like: Why are pydot.py (from where Graphviz is called) and dot_parser.py (from where PyParsing is called) separate? Does sharing an exception or exception hierarchy between the two fit in with that? Should we define the exception hierarchy in a separate file? What to do with the existing exceptions, which have some problems already? Naming issues, etc. etc.

@ankostis replied, also on 2020-05-01:

Package & module organization

I agree, the current organization of python files is "unusual", and complicates crafting a sensible exception hierarchy.
The majority of distributions in PyPi are either a single python-file (a module),
or a package (a folder with __init__.py package-module) containing more modules files inside.

If dot_parser.py us considered a "private" file, it' i's easy to retrofit the existing 2 modules into a more conventional format like that:

pydot/
  +--__init__.py  # all "public" objects must be defined/imported in this module
  +--dot_parser.py 
  +--base.py      # read below

The Exceptions should be definitely defined in the package-module.

It is tempting to copy-paste the whole pydot.py contents on the package-module
but i would recommend against it.

TIP1:
When a user imports a module further into a package, the package-module gets import
in its entirety - so if it is "heavy" it delays the import. That is important when
the different functionality is split in multiple submodules of the package.
Example:
A package contains 3 sub-modules: base.py, sphinx_extension.py
If base.py contains also code for irrelevant http-server communications,
whenever from <package> import sphinx_extension code is trun,
it has to pay the price for this never-to-be-used http-server functionality.

TIP2
The modules are split based on the 3rd-party imports. If a module is completely disparate,
it might become an "extras" in the setup.py.

Copy all in the base.py (or choose another name for this module, just my convention, try core.py),
and import from package-module only what is needed.

I do not have time to restructure the entire package, and it seems @ankostis does not either. So, for now, I just create this issue as a reminder/todo-item. Anyone, feel free to give your comments or work on this issue.

(With regard to the original questions of error handling and logging, I will update my PRs based on the current structure, while also trying to anticipate future changes that may come from a restructuring of the package & module organization. I will keep an eye on this issue and am willing to change my PRs as long as they have not been merged. After that, they will become part of the code to be restructured, just like all the other existing code.)
@peternowee
Copy link
Member Author

pydotplus, a fork of pydot that was active in 2014, went through a reorganization like this. Though it got some criticism, still may be worth having a look at how they solved it:

@peternowee
Copy link
Member Author

As did the nlhepler/pydot fork in 2013, which became part of pydot/pydot-ng:

@peternowee peternowee mentioned this issue Sep 25, 2020
Merged
@peternowee peternowee added this to the 2.0.0 (or later) milestone Oct 30, 2020
This was referenced Jun 25, 2021
Open
Open
peternowee added a commit to peternowee/pydot that referenced this issue Jul 12, 2021
@peternowee
Restructure project directory layout
ed751c9 
This commit brings the directory layout in line with current
recommendations by the Python Packaging Authority and others.

    Before                        After

    |-- setup.py                  |-- setup.py
    |                             |-- src/
    |                             |   `-- pydot/
    |                             |       |-- __init__.py
    |-- pydot.py                  |       |-- core.py
    |-- dot_parser.py             |       `-- dot_parser.py
    `-- test/                     `-- test/
        `-- pydot_unittest.py         `-- pydot_unittest.py

There are many opinions on what would be the best directory layout for
a Python project, particularly on whether or not to use an additional
`src/` layer. For example:

- https://blog.ionelmc.ro/2014/05/25/python-packaging/#the-structure
- pypa/packaging.python.org#320
- https://github.com/pypa/packaging.python.org/blob/7866cb69f1aa290a13c94da77cfb46a079cfcfa2/source/tutorials/packaging-projects.rst#a-simple-project
- https://stackoverflow.com/questions/193161/what-is-the-best-project-structure-for-a-python-application

Suffice to say, I can basically understand the arguments in favor of a
`src/` layout and do not find the arguments against it strong enough
not to use it.

Moved the dunder global variables (`__version__` etc.) to `__init__.py`
to ensure they remain available to the user doing `import pydot`. (They
would not be included in `from pydot.core import *` because their names
start with an underscore and I did not want to start keeping `__all__`
just for this.) A test for `pydot.__version__` is added to the
testsuite as well.

.... TODO: Something about separating parsing from core or not? ...

Discussed in pydot#171, pydot#230 and pydot#271.

Special thanks to Kostis Anagnostopoulos (@ankostis) for his advice.
peternowee added a commit to peternowee/pydot that referenced this issue Jul 12, 2021
@peternowee
Exceptions: Move to separate module, New hierarchy
f830c5b 
- Module structure following example from NetworkX and other projects.
  This adds an `import pydot` to `core.py`, which is cyclic/circular,
  but necessary to make the exceptions available. Cyclic imports are
  not uncommon and Python can handle this. Using the exceptions in
  pydot source code now requires prefixing `pydot.`, for example:

      raise pydot.Error("Message")

- Exception hierarchy changes. See ChangeLog and class docstrings in
  this commit for details. Additional background notes:

  - Removal of the unused exception class `InvocationException`: It was
    introduced in 842173c of 2008 (v1.0.2), but got in disuse when
    commits 9b3c1a1 and bc639e7 of 2016 (v1.2.0) fell back to using
    `Exception` and `assert` (`AssertionError`) again. If we ever need
    a custom class like this again in the future, it would probably
    have a different signature for context data (e.g. a DOT string,
    input and output), different parent class (or classes, e.g.
    `PydotException, CalledProcessError`) and perhaps a different name
    (e.g. ending in `...Error`), so no need to keep the old class
    around for that.

Further additions to the exception hierarchy are likely before the
final release of pydot 2.0.

Discussed in pydot#171, pydot#230 and pydot#271.
@peternowee peternowee mentioned this issue Jul 12, 2021
Merged
@peternowee
Copy link
Member Author

PR in #271.

peternowee added a commit to peternowee/pydot that referenced this issue Jul 30, 2021
@peternowee
Restructure project directory layout
c0ba69a 
This commit brings the directory layout in line with current
recommendations by the Python Packaging Authority and others.

    Before                        After

    |-- setup.py                  |-- setup.py
    |                             |-- src/
    |                             |   `-- pydot/
    |                             |       |-- __init__.py
    |-- pydot.py                  |       |-- core.py
    |-- dot_parser.py             |       `-- dot_parser.py
    `-- test/                     `-- test/
        `-- pydot_unittest.py         `-- pydot_unittest.py

There are many opinions on what would be the best directory layout for
a Python project, particularly on whether or not to use an additional
`src/` layer. For example:

- https://blog.ionelmc.ro/2014/05/25/python-packaging/#the-structure
- pypa/packaging.python.org#320
- https://github.com/pypa/packaging.python.org/blob/7866cb69f1aa290a13c94da77cfb46a079cfcfa2/source/tutorials/packaging-projects.rst#a-simple-project
- https://stackoverflow.com/questions/193161/what-is-the-best-project-structure-for-a-python-application

Suffice to say, I can basically understand the arguments in favor of a
`src/` layout and do not find the arguments against it strong enough
not to use it.

Moved the dunder global variables (`__version__` etc.) to `__init__.py`
to ensure they remain available to the user doing `import pydot`. (They
would not be included in `from pydot.core import *` because their names
start with an underscore and I did not want to start keeping `__all__`
just for this.) A test for `pydot.__version__` is added to the
testsuite as well.

A question that remains open for the moment is whether we should reduce
the weight of pydot "core" by splitting off functionality, most notably
the parsing functionality. Although that would reduce pydot loading
time, it would also make it more difficult for users to discover and
use that functionality. For details see:
pydot#271 (comment)
For now, things are kept as-is, namely that `pydot` always imports
`dot_parser`. This is now accomplished via `__init__.py` and `core.py`.

Discussed in pydot#171, pydot#230 and pydot#271.

Special thanks to Kostis Anagnostopoulos (@ankostis) for his advice.
peternowee added a commit to peternowee/pydot that referenced this issue Jul 30, 2021
@peternowee
Exceptions: Move to separate module, New hierarchy
90936e7 
- Module structure following example from NetworkX and other projects.
  This adds an `import pydot` to `core.py`, which is cyclic/circular,
  but necessary to make the exceptions available. Cyclic imports are
  not uncommon and Python can handle this. Using the exceptions in
  pydot source code now requires prefixing `pydot.`, for example:

      raise pydot.Error("Message")

- Exception hierarchy changes. See ChangeLog and class docstrings in
  this commit for details. Additional background notes:

  - Removal of the unused exception class `InvocationException`: It was
    introduced in 842173c of 2008 (v1.0.2), but got in disuse when
    commits 9b3c1a1 and bc639e7 of 2016 (v1.2.0) fell back to using
    `Exception` and `assert` (`AssertionError`) again. If we ever need
    a custom class like this again in the future, it would probably
    have a different signature for context data (e.g. a DOT string,
    input and output), different parent class (or classes, e.g.
    `PydotException, CalledProcessError`) and perhaps a different name
    (e.g. ending in `...Error`), so no need to keep the old class
    around for that.

Further additions to the exception hierarchy are likely before the
final release of pydot 2.0.

Discussed in pydot#171, pydot#230 and pydot#271.
This was referenced Dec 30, 2023
Merged
Merged
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Feb 6, 2024
py-dot: updated to 2.0.0
569d10d 
2.0.0 (2023-12-30)
------------------

Changed:
- Broken parsing caused by `pyparsing` updates fixed.
  With this, the pydot project rises from the dead.
- (Internal) CI revived by @ferdnyc.
  Modernized and clarified the development process.
  Testing is done against multiple Python versions.
- Reorganized package/module structure.
  The `pydot` package is installed as a directory now instead of as
  two modules:

      Before (pydot 0.x, 1.x)    After (pydot 2.x)

      site-packages/             site-packages/
      |-- pydot.py               `-- pydot/
      `-- dot_parser.py              |-- __init__.py
                                     |-- core.py
                                     |-- dot_parser.py
                                     `-- exceptions.py

  This is mostly an internal change that should go unnoticed by most
  users, especially those upgrading through `pip` or a software
  distribution. `import pydot` should work as it did before.
  Special cases:
  - `import dot_parser` no longer works. Change it to
    `from pydot import dot_parser` or see if you can use the wrappers
    `pydot.graph_from_dot_data()` or `pydot.graph_from_dot_file()`.

    **USER FEEDBACK REQUESTED**
    We assume pydot users do not often directly `import dot_parser`.
    If you do, please report your reasons, so that we can consider
    making it available again before the final release of pydot 2.0:
    pydot/pydot#230

  - If you use pydot from a (cloned) pydot source tree:
    - The pydot source modules moved from the top directory to
      subdirectory `src/pydot/`.
    - When using a `PYTHONPATH` environment variable: Append `/src`,
      e.g. `PYTHONPATH=~/Development/pydot/src`. If you need to switch
      between pydot 1.x and pydot 2.x, add both, e.g.
      `PYTHONPATH=~/Development/pydot/src:~/Development/pydot`
    - When using an editable install (development mode): Re-run
      `pip install -e .` from the top directory of the source tree to
      update the links.
  - For users of the test suite:
    - The test suite no longer refuses to run from the top of the
      source tree.
    - This makes the test suite option `--no-check` redundant. It has
      no effect except for printing a deprecation warning. It will be
      removed in a future major release (pydot 3 or higher), then
      leading to an error.
- Reorganized exception hierarchy:
  - New base class `PydotException`, derived from Python's `Exception`.
  - Pydot's `Error` exception class is now derived from `PydotException`
    instead of from Python's built-in `Exception` directly. Existing
    handlers should not be affected.
  - Exception class `InvocationException` was removed. It has not been
    raised by pydot since 2016 (v1.2.0).

- API (minor): Renamed the first parameter of the parser functions
  listed below from `str` to `s`. These functions primarily exist for
  internal use and would normally be called using positional arguments,
  so few users should be affected.
      push_top_graph_stmt(s, loc, toks)
      push_graph_stmt(s, loc, toks)
      push_subgraph_stmt(s, loc, toks)
      push_default_stmt(s, loc, toks)
      push_attr_list(s, loc, toks)
      push_edge_stmt(s, loc, toks)

Deprecated:
- Test suite option `--no-check`. See "Reorganized package/module
  structure" above.

Removed:
- Drop support for Python 2 and Python < 3.7.
  **USER FEEDBACK REQUESTED**
  ~~We are considering if pydot 2.0 should drop support for Python 3.5
  and 3.6 as well. If this would affect you, please leave a comment in
  pydot/pydot#268
  EDIT: This was decided to be done, with a lot of time passed since this entry.
@renovate renovate bot mentioned this issue May 4, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api discussion internals
Projects
None yet
Milestone
2.0.0 (or later)
Development

No branches or pull requests
1 participant
@peternowee