dsa-ou / algoesup

Algorithmic essays support: examples, templates, guides, library
https://dsa-ou.github.io/algoesup/
BSD 3-Clause "New" or "Revised" License
2 stars 0 forks source link

Enable `pip` installation for `algoesup` library #10

Closed densnow closed 8 months ago

densnow commented 9 months ago

Make algoesup installation via pip possible.

Some of the tasks include:

mwermelinger commented 9 months ago

Happy for you to assign yourself if you want the experience, otherwise can do it myself.

densnow commented 9 months ago

Yes I will have a go at this one if you don't mind. Although when it comes to uploading the package it will be best to use the account that is already set up, and was used for allowed etc.

Since this repo is not solely dedicated to the algoesup Python library, I think it's best to have the structure something like the following.

/
|-- algoesup/
|   |-- algoesup/
|   |   |-- __init__.py
|   |   |-- algoesup.py
|   |-- README.md  # Specific README for algoesup package
|   |-- pyproject.toml  # Configuration for the algoesup package
|-- Deepnote/
|-- docs/
|-- ...
|-- README.md  # README for the whole repo
mwermelinger commented 9 months ago

The structure looks good, thanks.

densnow commented 9 months ago

I did have the middle folder named lib but changed it after reading lib is sometimes the naming convention for storing Cpython libraries. But I guess lib is a fairly common name, so all good.

The @register_line_magic decorators (and the other magic decorators) mean that the module does not import into a standard python session without errors when installed with pip. Importing into an ipython session with a blank __init__.py file loads the regular python functions from the library, but does not register and allow the magics to work.

The "quick and cheap" fix for the above is to register all the magics and ipython events in the __init__.py so they are registered when the module is imported and __init__.py is executed. But, again you would not be able to load the module into standard python environment.

It seems the convention and the longer option is to separate any ipython specific code into a new module, and load it using the %load_ext <name of module> magic. This requires writing a load_ipython_extension(ipython) function in either the new module or the init file (need to do more reading) which is executed on the %load_ext magic being run.

mwermelinger commented 9 months ago

It's never as simple as it looks, is it? 😄 We need it to load in a normal Python environment because people may wish to use only the test or timing functions.

The nb_mypy extension we initially used has put everything in the init.py, including the load_ipython_extension but I think I prefer to keep things in the algoesup file.

For the moment, try to make one extension per linter to change the code the least possible, but later we can try having a single linter extension that allows turning on/off different linters.

densnow commented 9 months ago

Splitting the code into two modules seems to work.

The timing functions have been kept in a module core.py, the magics and associated files in a magics.py module. This allows the timing functions to be imported with import algoesup.core or from algoesup.core import ..., (meaning no errors in normal python environment) and the magics can be loaded using %load_ext algoesup.magics

@mwermelinger what do you think about this kind of solution?

densnow commented 9 months ago

I should add that using %load_ext algoesup.magics allows all of the magics i.e %allowed, %ruff and %pytype to be used.

mwermelinger commented 9 months ago

Sounds good, thanks. I'd have two modules time and test instead of core, which is not descriptive. The init file can then import everything from those modules so that users can import directly from algoesup import test, time_....

densnow commented 9 months ago

I've pushed the 10-enable-pip branch that I was working on. It is by no means finished there is still the README to write along with module docstrings and things like that, but the core functionality seems to be working.

lib is in the .gitignore by default. That kind of warned me off using the name so I just called the middle directory library

I will continue to get the little bits and pieces done, any comments are welcome in the time being.

mwermelinger commented 9 months ago

Maybe use src instead of lib. IIRC that's what poetry does when creating new projects from scratch with poetry init.

densnow commented 9 months ago

@mwermelinger when you say:

Move the instructions on using the library from the writing guide to new section(s).

I am assuming we are talking about a new section in the docs under the heading of "Library" or something like that?

Do we also need some kind of quick reference documentation that just quickly shows the function signatures and parameters?

mwermelinger commented 8 months ago

Yes, a new Library section that serves as reference, and with the usage examples from the writing guide.

densnow commented 8 months ago

@mwermelinger Do you know yet which plugin for mkdocs we will use to generate some of the documentation? What kind of adjustments are needed in the source code for this change? I am assuming we need to improve and expand the docstrings?

mwermelinger commented 8 months ago

The mkdocs branch is using mkdocstrings. No special adjustment needed other than docstrings must be strict Markdown: blank lines before lists, double-space to break lines, 4-space indentation. More info in the docstrings about the parameters is welcome anyhow.

densnow commented 8 months ago

Looking at the Python handler docs for mkdocstrings, it seems they use griffe to collect the documentation. Furthermore griffe can parse docstrings written in some of the popular styles. I think writing the docstrings in the google style could be a good option here.

mwermelinger commented 8 months ago

Feel free to use any style you want, although it's also fine to keep it simple for the moment, as the priority is to get it on PyPI.

densnow commented 8 months ago

Good point. I think this is probably a separate concern for a different issue!

mwermelinger commented 8 months ago

Fixed by commit c06c3dd1dfe832aa8f25d15a67f341ca74131f3f