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

Fix settings lineage #110

Merged
merged 5 commits into from
Jun 7, 2020
Merged

Fix settings lineage #110

merged 5 commits into from
Jun 7, 2020

Conversation

kirienko
Copy link
Owner

@kirienko kirienko commented Jun 7, 2020

This fixes adding a lot of extra newlines during simple changes in settings.py during the installation process (spotted by @maweki).

The problem was that by default print function obviously adds an extra \n in the end.

I also changed the inplace parameter from 1 to True for sake of readability.

@kirienko kirienko requested a review from maweki June 7, 2020 17:56
@kirienko kirienko changed the title Fix settings lineage [WIP] Fix settings lineage Jun 7, 2020
@kirienko
Copy link
Owner Author

kirienko commented Jun 7, 2020

Hold on a second, why it breaks our build now?

@maweki
Copy link
Collaborator

maweki commented Jun 7, 2020

Is this really best practice for coding paths during setup? I'd guess not. If we want to throw this part of the setup out and replace it by something somewhat maintainable, I am all in favor of it.

@kirienko
Copy link
Owner Author

kirienko commented Jun 7, 2020

@maweki I fully agree that this way of doing things might not be obvious. But python packaging is rather strange thing by itself, and we need to have at least something for right now. This doesn't fix #107 of course but this will do the installation a tiny bit less ugly.

Maybe we can kindly ask @takluyver to tell us about best practices 😏

@takluyver
Copy link
Collaborator

I think this looks OK as a small fix for a small issue.

I'm not aware of any best practice for this kind of thing. Python packaging is mostly designed around delivering libraries rather than applications, and the general pattern is that anything your code wants to access at runtime is delivered inside the package folder, and accessed using a relative path, e.g. with importlib.resources.

Python packaging has moved away from running setup.py install: pip will now try to use a wheel, and if one isn't available, it will run setup.py bdist_wheel to make a wheel, and then install from that. This is important because the build step doesn't know where the package will be installed. And installing a wheel doesn't run any code from that wheel. In short, this approach of rewriting paths at install time is doomed.

@maweki
Copy link
Collaborator

maweki commented Jun 7, 2020

In short, this approach of rewriting paths at install time is doomed.

Doomed and unmaintainable. Is there a way we can do it with relative paths? What would we need to do to fix this?

@kirienko
Copy link
Owner Author

kirienko commented Jun 7, 2020

BTW I tried to install gourmet from the package created by python setup.py bdist, and it didn't help either.

@kirienko
Copy link
Owner Author

kirienko commented Jun 7, 2020

Thank you Thomas!
If there's no other objections, I would merge this small fix and, I'd offer everyone to continue the discussion in #107

@kirienko kirienko changed the title [WIP] Fix settings lineage Fix settings lineage Jun 7, 2020
@kirienko kirienko merged commit 26fb644 into master Jun 7, 2020
@kirienko kirienko deleted the fix-settings-lineage branch June 7, 2020 21:19
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