bluesky / event-model

data model for event-based data collection and analysis
https://blueskyproject.io/event-model
BSD 3-Clause "New" or "Revised" License
13 stars 30 forks source link

Skeleton adoption branch 1 #245

Closed evalott100 closed 1 year ago

evalott100 commented 1 year ago

Closes #240

Converts event-model to use the DLS python skeleton, adding all the features other than the .vscode and the container, the following have been added:

The CI has been tested, and will fail only on flake8 and mypy since the master branch __init__.py does not yet have typing support.

This change will introduce to event-model the older commits to the DLS python skeleton - those which altered features which will be included in this PR.

evalott100 commented 1 year ago

CI now uses jsonschema version 2 and 3 in a matrix

coretl commented 1 year ago

Comment out mypy in a single commit, then restore it in #247

coretl commented 1 year ago

@mrakitin any other changes, or shall we merge as soon as @evalott100 has temporarily commented out mypy in CI in this branch?

mrakitin commented 1 year ago

By the way, I noticed pipx is used in the CI for generating sdist/wheels, but there is no mention of it in the requirements. Where do we get it from?

$ git grep pipx
.github/workflows/code.yml:          pipx run build
.github/workflows/code.yml:        run: pipx run twine check --strict dist/*
coretl commented 1 year ago

By the way, I noticed pipx is used in the CI for generating sdist/wheels, but there is no mention of it in the requirements. Where do we get it from?

As it is only used in CI, then we rely on it being preinstalled, like git: https://github.com/actions/runner-images/blob/main/images/linux/Ubuntu2204-Readme.md

mrakitin commented 1 year ago

As it is only used in CI, then we rely on it being preinstalled, like git: https://github.com/actions/runner-images/blob/main/images/linux/Ubuntu2204-Readme.md

Great! I was not aware it was pre-installed. It's a new tool for me, thanks for the details.

Don't you have scenarios when one would like to create the sdists/wheels locally? Maybe it can still be added to the dev requirements, but I have no strong opinion about it.

By the way, when I tried to create the distributions, it reported the version number 0.0.0... Is it expected?

$ pipx run twine check --strict dist/*
Checking dist/event_model-0.0.0-py3-none-any.whl: PASSED
Checking dist/event-model-0.0.0.tar.gz: PASSED
evalott100 commented 1 year ago

Made the fixes to this repo, and added fixes in the skeleton repo for the issues that were shared.

coretl commented 1 year ago

Don't you have scenarios when one would like to create the sdists/wheels locally? Maybe it can still be added to the dev requirements, but I have no strong opinion about it.

We always pip install /path/to/event_model if we want a local install, so I can't say I've ever used pipx locally. As we have a venv then pip install twine is easy to do, and it's unlikely many people would ever need to do it, so I'd err on the side of not including it.

By the way, when I tried to create the distributions, it reported the version number 0.0.0... Is it expected?

$ pipx run twine check --strict dist/*
Checking dist/event_model-0.0.0-py3-none-any.whl: PASSED
Checking dist/event-model-0.0.0.tar.gz: PASSED

That's unexpected, @evalott100 could you see if you can reproduce locally?

@mrakitin when this last issue is sorted, are there any others outstanding?

danielballan commented 1 year ago

Thanks for the revisions @evalott100. Where does the v0.0.0 issue stand?

evalott100 commented 1 year ago

Thanks for the revisions @evalott100. Where does the v0.0.0 issue stand?

I just did the build now and get the same 0.0.0 issue. I'll look into this Today!

evalott100 commented 1 year ago

@mrakitin The 0.0.0 issue is fixed now, was a simple case of including an empty [tool.setuptools_scm] in the pyproject.toml

mrakitin commented 1 year ago

Are black and flake8 still not in agreement?

event_model/__init__.py:283:89: E501 line too long (103 > 88 characters)
event_model/__init__.py:291:89: E501 line too long (103 > 88 characters)
event_model/__init__.py:292:89: E501 line too long (107 > 88 characters)
event_model/__init__.py:314:89: E501 line too long (94 > 88 characters)
event_model/__init__.py:331:89: E501 line too long (102 > 88 characters)
event_model/__init__.py:337:89: E501 line too long (90 > 88 characters)
event_model/__init__.py:1763:89: E501 line too long (101 > 88 characters)
event_model/__init__.py:1764:89: E501 line too long (103 > 88 characters)
event_model/__init__.py:1913:89: E501 line too long (95 > 88 characters)
event_model/tests/test_filler.py:376:89: E501 line too long (95 > 88 characters)
evalott100 commented 1 year ago

Are black and flake8 still not in agreement?

event_model/__init__.py:283:89: E501 line too long (103 > 88 characters)
event_model/__init__.py:291:89: E501 line too long (103 > 88 characters)
event_model/__init__.py:292:89: E501 line too long (107 > 88 characters)
event_model/__init__.py:314:89: E501 line too long (94 > 88 characters)
event_model/__init__.py:331:89: E501 line too long (102 > 88 characters)
event_model/__init__.py:337:89: E501 line too long (90 > 88 characters)
event_model/__init__.py:1763:89: E501 line too long (101 > 88 characters)
event_model/__init__.py:1764:89: E501 line too long (103 > 88 characters)
event_model/__init__.py:1913:89: E501 line too long (95 > 88 characters)
event_model/tests/test_filler.py:376:89: E501 line too long (95 > 88 characters)

@mrakitin They are in agreement now, however black doesn't format strings to be multi-line if they exceed 88 characters (which wasn't a problem with flake8 before changing the max line length). These were all just strings which needed to be put in multiple lines.