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

Make this runnable on its own and allow shallow folders #13

Merged
merged 3 commits into from
Feb 26, 2019

Conversation

nachtm
Copy link
Contributor

@nachtm nachtm commented Feb 22, 2019

Currently, this does not run (#12).

This PR adds a __main__.py file after working with @Bentleysb and @danielduhh to figure out the best approach. Now, this can be run like so from the root of the project:
python3 -m src.mqm --folderPath=/Absolute/folder/path.

In addition, I added a hotfix that @danielduhh created to allow this to run on a single shallow folder instead of a nested folder. As we improve the way this code is structured, this may change, but it gets the functionality we want for now.

One word of warning: there is currently an issue regarding the location of the created results folder. I'll make an issue and add a link here shortly. See #14

Copy link
Contributor

@danielduhh danielduhh left a comment

Choose a reason for hiding this comment

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

thank you!

Copy link
Collaborator

@Bentleysb Bentleysb left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Outdated
@@ -22,7 +22,7 @@ pipenv shell
2. run the program through applying <br />

```
python3 mqm_tool.py --folderPath [a absolute folder path] --maxDepth [maximum tree depth (default = 10)]
python3 -m src.mqm --folderPath [a absolute folder path] --maxDepth [maximum tree depth (default = 10)]

Choose a reason for hiding this comment

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

This is kind of an odd module to call...Can this not just be mqm and not src.mqm? With some restructuring?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be if you are in the src folder. It is just odd because we have the repo set up for publishing to pypl, but still want it to be executable from the root. If you were to install this as a package you could just call python3 -m mqm.

Choose a reason for hiding this comment

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

Mmm, I feel like ideally (and traditionally) the installation and all command line functionality for a module are called using the same name. This inconsistency is kind of confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - I'd rather not have to call src.mqm; how can we fix that ? @nachtm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a lot of reading, I think the confusion surrounding this is actually just a documentation/understanding issue on my part, and is a function of how the src layout is supposed to work. The advantage of this layout is that it is hard to use the local version without installing it -- we want to install to make sure that a) the code builds and b) the imports work for people who do install from PyPI for example.

But, I think we can do both (run python3 -m mqm and only run the code as installed), and I've updated the pipfile to reflect that. Give it a shot and let me know if you run into problems.

Taking this into account, there will be two ways to run this code:

  1. (eventually) pip install a release version from PyPI, and run python -m mqm --args.
  2. Clone this repository, activate the pipenv shell, and run python -m mqm --args from the root.

Is this intended? Do we need both? These are part of a larger discussion that I would like to have happen about the end goals for this project. If all we're doing is running a script, then I'm not entirely convinced we need to distribute through PyPI, and if we do distribute through PyPI but it's just an executable then I don't know if we need all of the scaffolding we have.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to get this thing up and running - thanks for making the updates

Copy link

@savannahostrowski savannahostrowski left a comment

Choose a reason for hiding this comment

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

Nice work @nachtm! Glad the chat helped. 🎉

@danielduhh danielduhh merged commit e0bc0ed into spatialdev:master Feb 26, 2019
@nachtm
Copy link
Contributor Author

nachtm commented Feb 26, 2019

Yes -- super helpful @savannahostrowski!

@nachtm nachtm mentioned this pull request Feb 26, 2019
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.

4 participants