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

Feature/packaging #15

Merged
merged 30 commits into from
Aug 7, 2023
Merged

Feature/packaging #15

merged 30 commits into from
Aug 7, 2023

Conversation

JeltevanBoheemen
Copy link
Contributor

This PR aims to make a full package out of sastadev.

  • Make an installable package of all the sastadev modules, rather than a limited subset.
  • Configuration
    • The implementation may seem weird, but I've exhausted options to do it in a different way that ensures both sasta and sastadev run well.
  • Proper test architecture
    • Many of the existing 'tests' are skipped because they do not produce a result, the result is dependent on locally available files, or they produce errors. We will aim to fix this at a later date.

For the individual reviewers:

  • @JanOdijk

    • This has some implications for how you write and use the code, but I have tried to keep these to a minimum. You will need to install the package locally (see README) to work on it. Also, mind the configuration, both setting and reading it.
    • Please try to follow the installation instructions, and let me know if anything is unclear or does not work. It is probably required that any sastadev references in your PATH are removed, so only the installed package is read.
  • @bbonf , @Meesch

    • Can you test this in auchann? Clone, install locally (e.g. `pip install /Users/my_name/projects/sastadev' and run the tests.
    • General code review. Most of the changes are moved files.

@JeltevanBoheemen JeltevanBoheemen added the enhancement New feature or request label Jun 22, 2023
@JeltevanBoheemen JeltevanBoheemen self-assigned this Jun 22, 2023
@bbonf
Copy link

bbonf commented Jun 22, 2023

This requires minor changes in mwe-query, which should be merged when the PR is done: UUDigitalHumanitieslab/mwe-query#9

def __init__(self,
ALPINO_HOST: str = 'localhost',
ALPINO_PORT: int = 7001,
LOGGER=logging,
Copy link

Choose a reason for hiding this comment

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

Might be better to use logging.getLogger('sastadev') instead of the root logger, giving more control to downstream users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I'll alter the main script a little bit to also work with that configuration.

print('Configuration initiated.')


settings = SastadevConfig()
Copy link

Choose a reason for hiding this comment

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

I would maybe prefer having no settings by default, and getting an exception that says I have to initialize sastadev before using any of its functionality. (Or at least a warning, in case you don't want to introduce breaking changes)
Otherwise it could be hard to tell whether something was imported before being configured.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this file deleted. The content is needed. Have you moved it elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is copied to src/sastadev/generatemacros.py. Somehow Git didn't pick register it as move but delete of the old file +create of the new file.

@JeltevanBoheemen JeltevanBoheemen merged commit 3f358e9 into develop Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants