Scrub UI - Integer Out of Range #2397 #2398
Conversation
Update scrub.py model Add django migration file
phillxnet
changed the title
|
@Hooverdan96 My apologies again for the slow response on this one. As indicated in the associated issue, I've popped this on into testing to get it 'out there' and under way. We can always cherry pick back to master for Stable release but as already discussed we have some pressures to re-establish testing and this is a nice clean pr to help kick off our new processes. Only think I'd like to comment on here was the commit message. They can help with quick diagnosis and the GitHub reference, alghough important, is secondary to the title text. So I think it's best to at least have a descriptive title plus the GitHub reference as a minimum. Again thanks for seeing to this fix. Much appreciated. I'll go try get my own back-log in order in-order that we get back to regular testing releases (is the hope). |
|
@phillxnet, yes, noted on the merge into testing first. On the commit message, I thought the reference to the issue itself provided some of the necessary insight. |
Agreed it does, but I think it's best if we initially put the info in the commit message itself, it is then transitioned auto (for a single commit anyway) into the pr so that cuts down on the pr prep work for the developer. And I'm a little wary of such links being broken in the future and the info being lost. So having it in the commit as the source of truth and the title being a single line abstraction of the commit's content is nice. That was all.
I see the issue title/text as the definition of the problem, and the pull/merge request title/text as how the issue was fixed, with reference to the issue text to keep down on duplication. But the commit title/message message, I think, should, at least to a developer, be self contained. And when creating a single commit pull request the commit title/test is transitioned directly to the pull request title/text so that simplifies that side of the contribution. So in short the issue is kind of user orientated with pointers to the developer approach likely, though not always of course. And the pull request is what was actually found and fixed which doesn't always add up to what was thought to be the problem in the issue. Nothing hard and fast of course, I just mentioned this as I'm always conscious of cloud cloud cloudy cloud stuff vanishing without a trace or future access or something breaking that renders a prior mechanism mute. So as long as we have self explanatory commit messages then all is not lost. You know how it is when you are looking up some old reference of a change and it's core details are held in a now non public repo, or forum, or whatever that no longer exists: there is a definite loss of info. We run this risk with our GitHub references, but as long as we explain ourselves in the git commit titles & messages we should be good and not stuck up a creek without a paddle of past info. Only guidelines and all contributions are welcome, I just noticed the commit title of "Fixes #2397" and tough I'd comment. Ideally we want to look down the commit titles only and have a rough idea of what they did, without reference to much else, but the content there-in is good, but a why it was done would also be nice. E.g. Commit submitted:
Suggested Commit:
That way, anyone quickly reviewing the git log, which is our only real source of truth, knows what this is concerned with (your original pr title which was super) more info on the titles implied failure (some data base range thing or other), and what was done to address it. That way when we review this in the future we have a fairly good grounding on where you were coming from with this fix and what you thought you were doing at the time. From personal experience it doesn't always look the same when one looks back after a while. Some projects get particular about grammatical stance etc. With corrections and guidelines on tense etc. This, in my opinion, is a waste of already very generous folks time and frankly nit-picky. And does'nt account for the commit message being in purhapse a 2nd or 3rd languages of the committer. If all folks can roughly glean what was done and why we are golden. But remembering that we don't need a re-description of the exact code change as the code changes themselves are their own truth on this. But an overview of why they were done is important. I.e. in our example here "... insufficient range .. Web-UI scrub report fail ..." As always I'm open to advise in this area and all other areas actually. And I try to make my time here as useful to future 'us' versions as possible. Hence the source of truth (nuggets at least) in the commit messages. Imagine if our GitHub repositories were deemed commercial and we were asked to front monies we simply didn't have to make them public. Or we were for example required/obliged to move to GitLab and our prior GitHub repos were deleted through inactivity!! Our git history (log) is essentially distributed, our GitHub record is not. We maybe need an entry in our Contributors doc entry for this approach. Our current guideline is here: I did more recently notice a failing in some commits where no clear line was entered directly after the title, that omission is actually a requirement to not have the entire git message be the title: and that is within some functions of git itself. So there are some easy failings to be had here. And I likely make several myself, you might not want to look at my next big testing pr for such failings actually (scope creep galore etc). Hope that helps P.S. this is a good reference for such things as the clear line after title that I mentioned as a requirement and some other odds and ends, but again, we do as we please as long as it's understandable and parsable by our selves and common tools: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-commit.html#_discussion |
|
@phillxnet - thanks, that all makes sense. I will try to abide by that framework. |
Fixes Issue #2397
see details in above listed issue.
Test Build on VM successful, survived reboots, etc.
Ran 217 tests in 29.519s