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.

Sign up for GitHub

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

Jump to bottom

Fix code smell issue in configuration #88

Closed
pydanny opened this issue Oct 8, 2023 · 4 comments
Closed

Fix code smell issue in configuration #88

pydanny opened this issue Oct 8, 2023 · 4 comments
Labels
help wanted Extra attention is needed

Comments

@pydanny
Copy link
Owner

pydanny commented Oct 8, 2023

Describe the bug

Code shouldn't know there might be tests. However, to get tests to work I hacked this into place.

Expected behavior

There should be no check for tests in regular code.
@pydanny pydanny added the help wanted Extra attention is needed label Oct 8, 2023
@RalphHuber
Copy link

Mind if I look at this? I started looking at it at pycon and have some ideas

@pydanny
Copy link
Owner Author

pydanny commented Oct 12, 2023

Yes! Go ahead!

@geoffbeier
Copy link
Contributor

Interesting, and related: test_warning_when_debug_false() depends on this smell as well; it only works if the fake config from the special case in activate() has been loaded first.

@RalphHuber I started looking at #7 and this smell spread into my patch idea for that one really quickly, so I wound up yak shaving before I thought too hard about it.

I have a quick idea that will get rid of the special case in activate() as well as making sure test_thing and test_warning_when_debug_false are truly independent of each other, and I will follow this comment with a very small PR.

I invite both you and @pydanny to please just close my PR and let me know it if it looks like it interferes with some more ambitious ideas you've been bouncing around for longer than the timebox I gave my yak shaving exercise.

geoffbeier added a commit to geoffbeier/dj-notebook that referenced this issue Oct 20, 2023
@geoffbeier
Remove special case for test harness from application code.
988f14b 
Addresses pydanny#88, at least partially.

This adds a context manager that ensures django settings are reset on exit, and has the two unit tests that call `activate()` use it.

It also adds two minimal settings modules that match the config those tests were seeing, and passes the names of those modules to `activate()` so that those are loaded using the same path as user-supplied settings modules.

I thought about using pytest-django's settings fixture, but the way `activate()` and `Plus` interact made that smell more than the original hack.
@geoffbeier geoffbeier mentioned this issue Oct 20, 2023
Merged
geoffbeier added a commit to geoffbeier/dj-notebook that referenced this issue Oct 20, 2023
@geoffbeier
Initial attempt at making path handling nicer.
39019ae 
At this point, for many cases `activate()` should just work.

Tested very lightly so far, and currently depends on a fix to pydanny#88.
@pydanny
Copy link
Owner Author

pydanny commented Oct 20, 2023

Closed in #120

@pydanny pydanny closed this as completed Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@pydanny @geoffbeier @RalphHuber