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

Update project setup #126

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

JuliaSprenger
Copy link
Collaborator

This PR switches from a setup.py approach to a pyproject.toml approach for defining package metadata and dependencies.
In addition it uses the src folder organization to separate source from other files.

For everything to still work smoothly I also had to make some minor adjustments to the code and the documentation. The corresponding documentation is here

@mdenker What do you think of this?

Copy link
Member

@mdenker mdenker left a comment

Choose a reason for hiding this comment

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

I think that's a great idea to change to toml. I made a few comments, but could test the install on my system yet due to bad internet. Will report back.

doc/environment.yml Outdated Show resolved Hide resolved
import importlib.metadata
# this need to be at the begining because some sub module will need the version
__version__ = importlib.metadata.version("odmltables")
VERSION = __version__
Copy link
Member

Choose a reason for hiding this comment

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

It seems VERSION is only used in two instances (to print the --version info for the CLI, and as a title for the GUI window). Maybe one could substitute a odmltables.__version__ in those instances and remove the VERSION variable alltogether?

pyproject.toml Outdated Show resolved Hide resolved
@@ -33,25 +32,25 @@
output, error = process.communicate()

import odmltables

VERSION = odmltables.VERSION

# reformatting requirements files to be automatically included in docu
Copy link
Member

Choose a reason for hiding this comment

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

Is this still something necessary, putting deps into the documentation?


import importlib.metadata
# this need to be at the begining because some sub module will need the version
__version__ = importlib.metadata.version("odmltables")
Copy link
Member

Choose a reason for hiding this comment

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

Will this actually also work for non-installed packages?

@@ -33,25 +32,25 @@
output, error = process.communicate()

import odmltables

VERSION = odmltables.VERSION
Copy link
Member

Choose a reason for hiding this comment

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

Maybe:

Suggested change
VERSION = odmltables.VERSION
VERSION = odmltables.__version__

import subprocess
import os

import tomllib

# If extensions (or modules to document with autodoc) are in another directory,
# add these directories to sys.path here. If the directory is relative to the
# documentation root, use os.path.abspath to make it absolute, like shown here.
#sys.path.insert(0, os.path.abspath('.'))
sys.path.insert(0, os.path.abspath('../src'))
sys.path.insert(0, os.path.abspath('../../python-odml/'))
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if this is needed? Seems like a hack of some sort....?

@@ -74,7 +74,7 @@ odmltables/gui/graphics/*.dvi
odmltables/gui/graphics/*.ps
Copy link
Member

Choose a reason for hiding this comment

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

All lines from 67-76 should have src/odmltables/gui/...

@mdenker
Copy link
Member

mdenker commented May 23, 2023

Update: I can confirm that I was able to install the plain and GUI versions using the new configuation.

JuliaSprenger and others added 2 commits May 23, 2023 22:10
Co-authored-by: Michael Denker <m.denker@fz-juelich.de>
Co-authored-by: Michael Denker <m.denker@fz-juelich.de>
@mdenker
Copy link
Member

mdenker commented Apr 19, 2024

Hi @JuliaSprenger, would you have time to solve the (very simple) merge conflict in the imports of odml_table.py so we can merge this PR? I could then take care of making a new release (maybe 1.0.2?) to resolve #133.

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.

2 participants