-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Initialize start time to a time and not None #6769
Conversation
@CAM-Gerlach You're quick. I was just coming back to add the labels. :-) |
Hehe, sorry for jumping the gun @csabella ! |
@CAM-Gerlach No need to apologize. It was just funny to me how quick you were. Are you sure you're not a bot? ;-) |
@csabella Haha, you just happened to catch me at a good time, when I had a short break during the day to triage the latest Spyder issues/PRs, etc, and had a relatively light issue queue. I'm pretty sure Carlos is actually quicker than I am, on average, since I am either at work or at school from ~9 - 5 CT each weekday and I don't have smartphone to able to keep up with it on the go.
Not yet, but sometimes I feel like one hehe. One of these days I'll get around to implementing one that does fuzzy matching and some other basic rules on new issue posts to automatically reply, tag, and close them...or better yet, uses machine learning to train itself automatically and continuously based on past issues and responses from the team. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
No test for this? |
The problem and the fix are simple and clear, and tests are expensive to make. I think it's better to invest our energies in testing more important things. |
I see...for my own part, I figure I spend somewhere around 5 or even 10x the time writing tests on some issues than fixing the few line issue itself, while having significant impacts on my own life and my ability to help fix more of the long queue of Spyder issues. Although, on the other hand, the tests I write have often caught problems with my initial solution and a number of related/unrelated bugs, and have certainly helped keep regressions from happening, so its all a balancing act, but perhaps I've taken it a bit too far in the former direction? |
You can submit a PR without tests and ask me or the other core devs if we consider a test is necessary or not. That's what sometimes the others do. For example, I asked @csabella to provide a test for PR #6543 but not for #6575. |
I think this is one of the positive side effects of adding unit tests. Even though it feels like unit tests could be a time suck at times, I think the results are worth it. And the nice thing is that they will be there the next time you make a change. You've been doing a lot of work on Variable Explorer, so the time you spend learning that codebase by changing it and adding tests will pay off on each subsequent change. Plus, as you said, you find bugs first. |
Thanks so much for all your input!
Yeah, that's the big draw for me @csabella —they are a cumulative improvement, such that each fix and corresponding test decreases the likelihood of a new or existing bug not being detected, permanently more or less, not just as a band aid. Sort of like how flying, despite being much riskier, is far safer than driving, because with each and every major bug (crash) new procedures, technology, etc. is adopted to minimize the chance of that crash happening again, to the point now that we hardly have any fatal crashes at all and the remaining have diminishing returns to fix. I really don't like band-aid solutions, and really like building something lasting and cumulative that in turn can be built on by others, and that's why I love open source and could never see myself working for a private company long term, at least that wasn't an open source friendly one.
Yeah, exactly—and not only that, I'm learning more and more about how to write the tests as well such that it goes much faster and with fewer issues I'm stuck on than before, and have also helped fixed a few of the testing issues on the Spyder end as well.
I guess I just read the policy and was asked for tests for each of my first few PRs, so just considered them a requirement every time where practicable. I suppose I could ask, but it would just shift the time sink to someone else so there would be no net gain, and personally I have a strong distaste of sucking up others time asking them for help if I am able to do it myself, though I am a bit of a sucker for others asking for help from me. A lot of it also is writing less overly complex tests and actually using constructs to reduce repetitive work and code smell, like fixtures and test cases, which doing so will help teach me. |
I see the benefits of writing tests, but I hate it so much nevertheless 🤣 |
@jnsebgosselin Its kind of a love-hate-love relationship for me. I'm attracted to the major benefits, but sometimes I dread all the work, and then once I start I like it too much and spend too much time writing overly fancy and comprehensive tests. Kind of like when I was a little baby with baths and showers..."Mommy, I don't want to get in the shower!!! [Ten minutes later] "Mommy, I don't want to get out of the shower!!!" |
Fixes #6767.
No reason to initialize self.t0 to None since time is already imported. Initialize it to a valid time and then allow the other methods to update it just as they have been doing. That way it will always have a valid value even if the kernel fails to start.