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

[wip] First pass proof-read of best practices lessons #42

Closed
wants to merge 7 commits into from

Conversation

eirrgang
Copy link
Contributor

Fixing typos, normalizing formatting, and noting areas that need additional attention.

eirrgang added 6 commits June 27, 2021 19:42
Ignore IDE files.
Before continuing, decide whether to remove content that is redundant with lesson 2.
@eirrgang eirrgang marked this pull request as draft June 27, 2021 18:51

> ## Packages and modules
>
> *TODO: Rewrite. Separate discussion of packages vs. modules from discussion of importable entities and scoping.*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this to do item. You can see the information the Python documentation has on modules here - https://docs.python.org/3/tutorial/modules.html

Python has a way to put definitions in a file and use them in a script or in an interactive instance of the interpreter. Such a file is called a module; definitions from a module can be imported into other modules or into the main module (the collection of variables that you have access to in a script executed at the top level and in calculator mode).

A module is a file containing Python definitions and statements. The file name is the module name with the suffix .py appended.

I think the definition given here is correct and also appropriate for the level intended. Can you clarify what you'd like to discuss in a rewrite?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of https://docs.python.org/3/glossary.html#term-module vs. https://docs.python.org/3/glossary.html#term-package
and https://docs.python.org/3/reference/import.html#regular-packages

Further muddying the waters are overloaded usages like https://packaging.python.org/glossary/#term-Distribution-Package and related terms.

There is a relationship between the filesystem layout and the import system that is really important to convey clearly and concisely. But it also seems important to recognize that what I import is a "module" (optionally, something nested in a modular namespace), whether or not that module (or submodule) is implemented as a "package".

It makes total sense to cite a more thorough outside reference like that tutorial, as long as the material presented in this lesson doesn't introduce confusion with respect to terminology at python.org.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eirrgang I'm sorry I can not fully grasp your idea. Maybe you could show a simple example of how the following Package and Module box should be changed?

Packages and modules

What 'packages' or 'modules' are in Python may be confusing.
In general, 'module' refers to a single .py file containing Python definitions and statements. It may be imported for use in another module or script. The module name is determined by the file name. A function defined in a module is used (once the module is imported) using the syntax module_name.function_name().
'Package' refers to a collection of Python modules. The package may also have an __init__.py file.

To read more about Python packages vs. modules, check out [Python's documentation].

In my opinion this explanation already fulfill its purpose as it is just a short explanation (or keynote). And the importable entity and scoping is there to explain how your module used in real life, so the student could understand the connection between what they create and the consequences when they are using them. Surely there are more than one way to define Python package but IMHO making it too comprehensive could lead to confusion among students.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making it too comprehensive could lead to confusion among students.

Certainly, that is a concern.

It is worth a try to see if a few words could be chosen more carefully. I'll give it a try at some point. Maybe the best and easiest thing is to just review 6-6.1 of the Python hosted tutorial (though that doc might get updated less frequently than this workshop material. ;-) )

I'll try out this Jekyll thing, though. Maybe some side-bars would ease my concerns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, what is so great about this workshop material is that it reflect the cms-cookiecutter. For example there are 2 notable changes in cms-cookiecutter since last year, the first one is switch autoformatting from yapf to black, and second one is the CI migration from Travis to Github Action. And this workshop material keep up with those changes. Which is awesome.

Yes, I can relate to the lack of sidebar, and it is hard to get used to. But it uses the template from Software Carpentry, and I found that it is not simple to edit the appearance.

@@ -205,11 +207,14 @@ $ cd molecool
### The `__init__.py` file

The `__init__.py` file is a special file recognized by the Python interpreter which makes a directory into a package. This file can be blank in some cases, however, we will use it to define how the user interacts with the functions in our package.
*TODO: Cite section on defining the interface, where we can also mention `__all__` and `_` prefixed names.*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be more appropriate in the section "Deciding Package Structure".

Copy link
Contributor

@radifar radifar Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe what he means is adding hyperlink to __init__.py in Deciding Package Structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, cross-linking with the later more thorough discussions would be good. The terseness of the description here seems appropriate, but

  • it shouldn't imply that this section completely describes how we define the public interface, and
  • references to public interface in Deciding Package Structure (or elsewhere) should include conventions on import behavior, documentation visibility, and linter semantics w.r.t. __all__ and underscore-prefixed names, at least in passing.

~~~
"""
molecool
A Python package for analyzing and visualizing xyz files. For MolSSI Workshop.
Analyze and visualize xyz files.
Copy link
Member

@janash janash Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section will be autopopulated based on answers to the cookiecutter prompts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it should be consistent with the generated material.

Is the docstring intentionally deviant from the PEP257 guidelines? Such as to make a point in the later lessons? If not, it seems like we should avoid mixed messages and

  • put a blank line between the first and second line
  • use the first line for a concise description according to PEP257 and numpy docstring conventions that will look good when formatted with pydoc or help(). This could mean just removing the molecool\n first line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eirrgang This docstring is automatically generated by the cookiecutter-cms. Therefore the above example comes automatically from the following template. However, I think it is still possible to add this material on Docstring section in Python Coding Style.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To develop this package, we will want to something called a developmental install so that we can try out our functions and package as we develop it.
To develop this package, we will want to use what is called "development mode" or an "editable install" so that we can try out our functions and package as we develop it. We access development mode using the `develop` command to `setup.py`, or the `-e` option to `pip`.

*TODO: Note that "editable" install is not (yet) standard and may even go away in the future.*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a reference for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I know calling setup.py directly (for install, test, etc.) is deprecated. Pip comes bundled with Python 3.4+ so let's stick with pip. And as a side note from what I learned, Poetry install your local package in editable mode as default, CMIIW.

@eirrgang could you elaborate more what does it mean by "not (yet) a standard"? From what I know there are lots of way to develop and test your project depending on what kind of project that you working on. For example in Web app framework like Django you can use the demo server. Or you can also directly deploy your code to a docker container every time you make the change. But I think installing in editable mode gives the freedom to check the API, or function/module without having to uninstall and install the package that we develop every time we make the change, which is very practical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There have been rumblings about deprecating non PEP517/518 behaviors, and I was thinking of the note at https://setuptools.readthedocs.io/en/latest/setuptools.html#setup-cfg-only-projects

But it is probably just something to keep an eye on, and doesn't warrant an update to the material at this time.

setuptools (and pip) are presenting an increasingly unified and standardized approach with the PEPs (wheel vs. egg, etc). We should definitely stick with pip. But you can't read documentation about Python packaging without coming across references to setup.py and distutils, and the terminology can get muddled. develop and "Development Mode" are common in the setuptools docs. -e and "editable installation" are pip terms, FWIW.

Copy link
Contributor

@radifar radifar Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eirrgang Yes this is very true, I've been reading Bernat Gabor's posts and watching some of his videos related to PEP517/518. From what I know there is a traction to move from setup.py to pyproject.toml. Few tools such as Flit and Poetry began to adopt this. And I also personally find these tools (especially Poetry) promising, but it will take some time before it is widely accepted in Python community.

As for now, IMHO I think it is still best to go with pip install as the PEP517/518 implementation is still work in progress and yet to be matured.

Edit note: added Pyproject.toml reference on Stackoverflow

@eirrgang eirrgang changed the base branch from gh-pages to july-2021 July 2, 2021 07:42
@eirrgang eirrgang closed this Jul 16, 2021
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.

3 participants