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

Integrate preprocess scripts #742

Merged
merged 11 commits into from
Jan 21, 2025

Conversation

ChristosT
Copy link
Collaborator

@ChristosT ChristosT commented Jan 9, 2025

This MR integrates the preprocessor scripts into the main hexrd executable via anew subcommand.
Usage:

hexrd preprocess <profile>

for now we have two profiles {eiger,dexelas}. Each profile comes with its own flags.

Adding a new profile boils down to creating a new class derived from PP_Base that holds the logic of the preprocessor see hexrd/preprocess/preprocessors.py and creating a new profile that holds the cli flags which be done by deriving from HexrdPPScript_Arguments see hexrd/preprocess/profiles.py. The new profile will then become available as an option in hexrd preprocess.

@pep8speaks
Copy link

pep8speaks commented Jan 9, 2025

Hello @ChristosT! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2025-01-21 16:29:10 UTC

@ChristosT ChristosT force-pushed the integrate-preprocess-scripts branch 3 times, most recently from 993943a to d2af8df Compare January 9, 2025 23:40
@ChristosT ChristosT self-assigned this Jan 9, 2025
@ChristosT ChristosT requested a review from psavery January 9, 2025 23:41
@ChristosT ChristosT force-pushed the integrate-preprocess-scripts branch 2 times, most recently from 834ad77 to 57be82f Compare January 16, 2025 19:20
Using a list of tuples forces the dumper to add type annotations,
while using a list of list avoid this issue.
Eiger should accept only one file provided via absolute_path while Dexelas
series of files  constructed using command line arguments
The latter is not a subset of the former. A new class is introduced
Chess_Arguments that is a superset of both
Missing types found using
mypy hexrd/preprocess/  --follow-imports=skip --disallow-untyped-defs
mypy hexrd/cli/preprocess.py  --follow-imports=skip --disallow-untyped-defs

using mypy --version
mypy 1.14.1 (compiled: yes)
Arguments set to None in argument profiles will be treated as required
positional arguments.  This behavior matches the original scripts.
@ChristosT ChristosT force-pushed the integrate-preprocess-scripts branch from 57be82f to 274e1f8 Compare January 21, 2025 16:29
Copy link
Collaborator

@psavery psavery left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Christos!

@psavery psavery merged commit 150108b into HEXRD:master Jan 21, 2025
6 checks passed
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