Setup

[jkbecker]

A setuptools-based installation script as well as the required re-structuring of the entire project.

See updated README.md for the new installation instructions once this is in effect.

[jkbecker]

Just some mental notes of what doesn't work yet

• Test setup
• getting the config file from the correct location based on where the package installed it deferred to config.yaml should not be searched based on the current directory. #84
• Clarify if MANIFEST.in is even needed - the files it contains don't seem to be copied into the installed package
• Probably more

[altendky]

Overall, it seems a pretty tidy transition as it should be. Thanks! Don't let the quantity of minor comments etc express otherwise. And yeah, in work etc. I prefer to share early and often in case there are significant issues that should be addressed early. Handily, I don't think there are any here.

I'll note here that this includes the changes in #60. If this is merged first it is accepting both changes. I don't have a problem with #60, just pointing it out.

MANIFEST.in is for the sdist, IIRC. If you specify include_package_data = True then it will make it into the wheel as well and thus the installation. IIRC... I think this may also replace [options.data_files]. But it's bin awhile since I've fiddled with this and it's one of the areas where there are many ways to do it and most have gotchas.

Maybe https://pypi.org/project/appdirs/ and a CLI override option for config file locations? Though this strikes me as maybe a separate issue unless installation breaks the existing functionality? I should go see what it is.

Maybe src/plotman/plotman.py should be core.py or something? Module paths like plotman.plotman strike me as a bit odd.

I would avoid encouraging people to activate the environment when running. There's plenty of room for personal preference on whether to activate or not but from the perspective of supporting people it ends up adding hidden global state in $PATH that you have to debug. Whereas if people send you a traceback with venv/bin/plotman then you know they are running the plotman in the env.

Looking good for a first share. I think this is one of the first things I poked @ericaltendorf about getting done with plotman. Good to see it happening!

[altendky]

@jkbecker, for getting the config from the 'right place', I think that can be a separate PR? This is already a restructure so keeping it focused is more valuable than normal. Also keeping it from getting held up on other changes is good. Unless I'm misunderstanding and the config location is particularly affected by this? From a quick search it seems to just be looking in cwd right now.

[jkbecker]

The main reason I wanted to couple those topics is: Once plotman is properly installed, it can be launched with the plotman command from anywhere, but the config.yaml references in the code are dependent on the current working directory. Essentially this will break if you're in the wrong directory, and as soon as you have a globally available command, that's not intuitive.

That said, considering I'll probably work on that next anyways, we could separate it as well?

There are some considerations when it comes to "correct" config file location handling.

• Typically you'd want the default location to be in appdir's user_config_dir
• To satisfy existing users we may want to attempt to read the config.yaml in the current directory with higher priority (?) (if it is in fact a plotman config.yaml - could be dangerous to just ingest any file with that name that happens to be in the working directory...
• With highest priority, plotman should also respect path overloading via a variable such as PLOTMAN_CONFIG=~/config.yaml plotman interactive if desired

Does this make sense? Am I missing a special case? Happy to deal with this in a separate PR if it makes the individual contributions more manageable, either way is fine for me.

[altendky]

Agreed on all points. I'm not sure if you said this indirectly or not, but config.yaml may just be a bad name at this point. Certainly understandable how it came to be though. :] I'd tend to separate the two just to avoid holding one up on the other and to have simpler to track reviews. Won't be making a fuss either way though.

[altendky]

I'll commit the minor tweaks tomorrow after reviewing my own review at a more appropriate hour. @ericaltendorf do you want to discuss the effects of this on users before merging to develop? No more launching as ./plotman.py and such. Or, is the addition to the README sufficient?

Thanks again @jkbecker, definitely happy to have this change. :]

[ericaltendorf]

I think the addition to the README should be sufficient, along with an announce on the plotman keybase channel. I guess improving the config.yaml situation will be a separate bit of work?

(FWIW what I've been doing so far is creating a sibling directory (e.g., plotman-run/) to the plotman/ git local repo, putting a symlink to ../plotman/plotman.py in there and a copy of config.yaml that I modify with my local settings.)

[altendky]

I guess I may as well draft the keybase message here. Suggestions and corrections are of course appreciated. Or, you could handle this if you want, @jkbecker. :]

---

Hi all. @jkbecker has kindly reworked the project structure of plotman for us so that it will be a properly installable Python package. This is being merged into the development branch. If you are using the main branch, this does not yet affect you. The new setup process has been documented in the readme. While this specific change does not change how plotman finds the config.yaml it would be understandable for some to be unsure where to put it. plotman still looks for config.yaml in the current working directory. This is the directory you are in in your shell when you run the program. You can keep your config wherever you wish.

Now that I've mentioned how the config.yaml is found presently, I'll go ahead and note that there has been some talk of changing that. The topic is ticketed as #84.

https://github.com/ericaltendorf/plotman#installation
#84

[jkbecker]

Looking good @altendky, thank you! I prefer not to announce it myself, as I'm not previously associated with the project. Don't want this to look like an intrusion ;)

❗ One detail just came to mind: If people just read the README on GitHub and pip install as suggested, they will actually have no config.yaml on their system at all! We're not covering this edge case.

Or are we assuming (for now), that people installing the development branch are existing users and somewhat more experienced with how Plotman works? At the very least, #84 should be addressed before merging to main.

[altendky]

It's not like I have some long standing involvement with plotman either. :] But anyways, good points. I'll add some wording both places on how to get a copy of the configuration for now. I'm good with also getting some configuration location changes in before a 'release' (I think that's what merging to main is?).

[jkbecker]

Hm, random idea: Maybe it would be a good idea to merge the current development branch progress to main, before merging setup into development?

I feel like there are a lot of small improvements in development that could be helpful now, people are complaining about bad GUI plot handling almost every other hour for example. Merging setup onto development would just make merging develop on main a much bigger change, right?

[altendky]

The request for more commentary on config.yaml is worthy of being a requested change I think. Even though I plan on doing it.

#61 (comment)