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

PLEP7: top level package structure #26

Merged
merged 67 commits into from
May 15, 2020

Conversation

rocco8773
Copy link
Member

@rocco8773 rocco8773 commented Oct 29, 2019

This PLEP is focused on:

  1. Defining the top-level sub-package namespace for PlasmaPy
  2. Defining the general scope of each top-level sub-package

The goal is to:

  1. Keep the top-level namespace of PlasmaPy from getting bloated
  2. Help guide developers/contributes on where to place their code

This PLEP was initially outlined by the PlasmaPy Coordinating Team at the 2019 61st APS-DPP Meeting.

P.S. I will remove the "draft" label when I'm satisfied with the PLEP and ready to merge it.


Still need to...

  • Start Zenodo upload to reserve DOI (https://doi.org/10.5281/zenodo.3774573)
  • Publish to Zenodo
  • Update ./README.rst with DOI
  • Update PLEP DOI field
  • Update PLEP date last revised field with accepted date
  • Update PLEP status field with accepted when accepted
  • Update linked PRs under section Issues, Pull Requests, and Branches when PRs are merged (see issue Update PLEP7 with PR and Issue links #32, the PLEP will be updated with links once the implementation is complete)

I assumed you didn't actually want to change `plasmapy.simulation` to `plasmapy.simulations` plural, so I reverted that.
PLEP-0007.rst Outdated Show resolved Hide resolved
PLEP-0007.rst Outdated Show resolved Hide resolved
@StanczakDominik
Copy link
Member

I went ahead and did a few initial comments, and oh dear, the tables broke them 😆 The Files Changed view will be a better choice for viewing them.

@rocco8773 rocco8773 added Status: Implemented Status: Voting PLEP pull requests that are being voted upon and removed Status: In preparation labels May 3, 2020
Copy link
Contributor

@SolarDrew SolarDrew left a comment

Choose a reason for hiding this comment

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

LGTM bar a few minor comments 👍 Thanks for all your work on this @rocco8773 et al

PLEP-0007.rst Outdated

If new code does NOT fall within the current framework, then this PLEP
needs to be modified/updated accordingly before any new top-level
sub-packages are created.
Copy link
Contributor

Choose a reason for hiding this comment

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

Or, presumably, the code should be ajusted to fit this framework?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reworded this a bit to indicate that amending the PLEP should be the last resort option. I assumed that would happen by default, since it will always be easier to conform code than to amend the PLEP and get it accepted.

This statement is here more so for the off chance that we overlooked something that should be a top-level package.

PLEP-0007.rst Outdated Show resolved Hide resolved
+-----------------+-------------------------------------------------------------+
| scope: | | A collection of tests for top-level modules (i.e. |
| | functions and classes defined in top-level ``.py`` |
| | files). |
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely we also want to test functions and classes defined in subfolders of these modules?

Copy link
Member

Choose a reason for hiding this comment

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

I thought folders of .py files were subpackages, and if that's the case, the current wording makes sense - there's no other place to put tests for .py files in the main plasmapy directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the python lingo, .py files are modules and folders are sub-packages.

@SolarDrew Yes, sub-package tests should still be done within their respective directory.

Choose a reason for hiding this comment

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

Think of this as a place for testing the interplay of various sub-packages. Will using dispersion capability in simulation break anything? Will using formulary inside analysis break anything?

Comment on lines +29 to +41
**Contents**

* `Abstract`_
* `Detailed Description`_

* `Why?`_
* `Top-Level Sub-Packages`_

* `Extensible Packages are Exempt`_
* `Implementation`_
* `Issues, Pull Requests, and Branches`_
* `Backward Compatibility`_
* `Decision Rationale`_
Copy link
Member

Choose a reason for hiding this comment

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

As an aside, maybe we should add a table of contents to the PLEP template. That way, future PLEP writers would have a draft table of contents ready.

@rocco8773 rocco8773 merged commit bd1bc1e into PlasmaPy:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Implemented Status: Voting PLEP pull requests that are being voted upon Type: Standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants