|
The only reason we do everything via pull-request is that this enables us to keep the master from direct changes (in case that the change breaks everything apart)...
So you can not commit directly to the master, only via pull-request, however you can approve yourself... (and the pull-request automatically triggers a build, as a condition to the merge)
"The only place where Success comes before Work is in the dictionary." Vidal Sassoon, 1928 - 2012
|
|
|
|
|
It's just another tool that can either be used to great effect or as unnecessary clutter. Depends on how the source control is managed.
I worked on a project where pull requests were used for a couple purposes:
1) to ensure merges into critical branches like main-development, production, and releases are explicitly done (not accidental),
2) as a form of code review where a pull request had to be accepted by at least one peer (you couldn't ok your own PR) after filling out a basic checklist (new tests pass, target branch tests still pass, merge is a fast-forward, and a couple others),
and 3) guarantee that git best practices are followed to avoid elephanting the history of the important stuff like ensuring you've merged main-dev into your branch and confirmed nothing broke before fast-forward merging back into main-dev. Fixing a merge problem on an important, shared branch, especially if it isn't caught until a couple merges later, is a nightmare.
It also serves as a discussion/checklist history so if anything does go catastrophically wrong, you not only know who requested the PR but who approved it and can educate them on what went wrong so it doesn't happen again.
You don't really get any of that with plain merges and I'd rather deal with PRs than paperwork. That's my 2 cents. Also I agree with everything Kornfeld said 
|
|
|
|
|
not one piece of code change should get merged to master until it has been looked at by another human being via a pull request. end of story.
|
|
|
|
|
Let me differ... No one of piece of code should be merged into the master until it passed some minimal test, no human intervention is necessary at any case...
"The only place where Success comes before Work is in the dictionary." Vidal Sassoon, 1928 - 2012
|
|
|
|
|
Let me tell you how we do it:
1.) developer, well, develops
2.) developer tests
3.) PR
4.) PR approved, merge to master
5.) QA tests, UAT by client if needed.
6.) Prod
It has been my experience that inflated egos do not jive well with code reviews(PR Reviews). To be honest, it took some time for me to get past my ego, in order to let someone else, whom I may not like, look at my code, and critique it, in the betterment of the product, not my ego.
In summary, get over yourselves.
|
|
|
|
|
It is not me - actually do not care who sees my code...
From above they decided that we have no time-to-spare to do code review...
"The only place where Success comes before Work is in the dictionary." Vidal Sassoon, 1928 - 2012
|
|
|
|
|
Kornfeld Eliyahu Peter wrote: From above they decided that we have no time-to-spare to do code review.
I have quit software shops in the past because of this mantra. If you can, I would advise the same. If you can't, then just know that this is not the shop for you. IMHO.
|
|
|
|
|
I thought the same, but working on lqarge projects with 120+ people working on it 24/7 I appreciated the better traceability Pull Request give, because there are less of them.
Uusually we have tons of commits, hundreds of pushes and dozens of pull requests. In the PR we can / have to attach the code static analysis, link the issues related (if any) and have it approved by one or more people.
The integrator does not have to enter knee deep in our commits, he just takes the pull request, merges it and sends to validation. This allows for easier tracing of "approved, definitive changes" and easier regression tracking.
On smaller projects it may be too much, but you never knwo when a project blooms (or explodes).
GCS d--(d+) s-/++ a C++++ U+++ P- L+@ E-- W++ N+ o+ K- w+++ O? M-- V? PS+ PE- Y+ PGP t+ 5? X R+++ tv-- b+(+++) DI+++ D++ G e++ h--- r+++ y+++* Weapons extension: ma- k++ F+2 X
|
|
|
|
|
It works well for code review 
|
|
|
|
|
The problem with not putting everything as a PR is how to define it, where to draw the line. You can't say "if it's only one change" or "only one line" don't bother with a PR as that one line\change could have a devastating impact. If you say "use common sense" then one man's common sense if another man's introduction of major bugs.
It's just easier to say everything needs a PR.
|
|
|
|
|
Pull requests assume:
1. there are other developers to review the code
2. said developers are qualified to review the code
3. said developers have time to bother with the review
And what really is the point? What, am I supposed to figure out what arcane git command will let me merge the pull request into my local code base so I can actually review it properly, ie., test, debug single step, etc.?
Meh. The only purpose for pull requests that I've ever found is to REJECT them. Particularly useful on open source projects where you get complete idiots making code changes.
|
|
|
|
|
Yea I feel like it...
Particularly most code review I just glaze at them and approve... Or, in the case of this team mate I mentionned, I feel like I would like to reject them all.. but well, it ain't going nowhere...
|
|
|
|
|
A pull request is a GitHub thing not git. So you would just pull the branch like normal to test.
git pull origin branch
git checkout branch
ant test
Then go to GitHub, write any descriptive info you need/want to, click submit review/request changes/etc, and click merge pull request if appropriate. If you want to go full automation TravisCI has easy built-in integration with GitHub to automate builds and tests on each pull request so you don't even need to use the commands above. It'll show the results in the PR and re-run if the PR branch is updated.
Of course if you're the only dev and will always be the only dev then any tool is just personal preference.
|
|
|
|
|
Some self praise because I implemented something even a lot of professional MIDI products don't have - Tap tempo / MIDI tempo sync, while recording, plus a demonstration app that allows you to tap tempo with your computer's keyboard.
It was quite a bit more difficult than I anticipated, because wrangling with different devices requires experimentation, and you don't want to clog the limited MIDI bus with never ending sync messages, but on the other hand if you send the minimum needed for timing (2 in theory) a lot of devices won't set their tempo from it immediately so it needs some finagling to get right. It's actually kind of complicated, logicwise. It took me the better part of the day to write the tap tempo demo as well, because figuring out the spec and the quirks took a lot longer than expected
Also god bless MIDI-OX. I should look it up again and see if i can pay for it. It's worth good money.
Anyway, my Midi dll now supports MIDI features even some pro tools don't have. Almost. I have actually another day or so of work on it, getting another half of it implemented using high resolution timers so it can send as well as receive them.
Another thing that's cool about it, is it's transparent. It converts the MIDI realtime sync into MIDI tempo change messages - usually only present in files, not over the wire - so a recorded sequence can automatically have the appropriate tempo changes in it for saving. And also my API supports receiving tempo change messages as well so it respects them even though they only come from files. It makes playing files easier.
It's kind of a convoluted contraption of sorts, but then MIDI is sort of kludgy and a well designed MIDI widget will handle the weird bits gracefully.
Anyway, yay me. This is a bear, but I'm almost out of the woods.
Real programmers use butterflies
|
|
|
|
|
Congratulations!
|
|
|
|
|
Well I spoke a little too soon. Turns out MIDI tempo sync isn't very accurate, at least from .NET, the MIDI drivers are not calling it accurately enough in terms of timing, at least by the time it gets to the .NET code (through all the P/Invoking through the callback and whatnot)
With MIDI-OX the results are better - my tap tempo app pretty reliably transmits the MIDI timestamps on time and MIDI-OX receives them on time so i know it's not the MIDI driver itself.
I've cranked the thread priority of my app, even though that shouldn't matter, and I made sure the particular message short circuits the regular handling and goes through a special abbreviated codepath to speed things up. Still it's off by a BPM or two in either direction. 
I guess I'll leave it in because it kind of works, but also because when I sort out the issue I'll have the rest of the code in place. I wish I had a better solution.
Unfortunately, there's precious little documentation on using the low level MIDI API from windows
Real programmers use butterflies
|
|
|
|
|
I've actually thought about adding such a feature to my MIDI sequencer. One item you might be overlooking is I don't believe Windows sends keyboard keystrokes to your app with a high-resolution timestamp. I think it relies on SendMessage, which can wait in the Windows que until your app gets its timeslice. That could explain your variation (~20ms, I think, although boosting your thread priority might reduce that a bit).
|
|
|
|
|
I'm actually doing it from the console, and I've checked the output with MIDI-OX. It's fine.
It's receiving that's giving me trouble.
Real programmers use butterflies
|
|
|
|
|
I don't understand. The console is not a real-time window in Windows, to my knowledge. But you say that side is working, so...
Good luck with it.
|
|
|
|
|
const int MIN_TEMPO = 50;
const int MAX_MSG_COUNT = 100;
Console.Error.WriteLine("Press escape to exit. Any other key to tap tempo");
using (var dev = MidiDevice.Outputs[1])
{
dev.Open();
long oldTicks = 0;
var amnt = 0d;
var next = 0L;
var dif = 0L;
var msgCount = 0;
while (true)
{
if (0 != oldTicks)
{
var dif2 = (_PreciseUtcNowTicks - oldTicks);
var tpm = TimeSpan.TicksPerMillisecond * 60000;
var amnt2 = tpm / (double)dif2;
if (amnt2 < MIN_TEMPO)
{
oldTicks = 0;
Console.Error.WriteLine("Tap tempo timed out for tempo less than " + MIN_TEMPO + "bpm");
}
}
if (Console.KeyAvailable)
{
if (ConsoleKey.Escape == Console.ReadKey(true).Key)
return;
if (0 == oldTicks)
{
Console.Error.WriteLine("Tap Started");
oldTicks = _PreciseUtcNowTicks;
}
else
{
dif = _PreciseUtcNowTicks - oldTicks;
var ts = new TimeSpan(dif);
var ms = ts.TotalMilliseconds;
var tpm = TimeSpan.TicksPerMillisecond * 60000;
amnt = tpm / (double)dif;
oldTicks = _PreciseUtcNowTicks;
Console.Error.WriteLine("Tapped @ " + amnt+"bpm "+ms+"ms");
next = _PreciseUtcNowTicks + (dif/24);
dev.Send(new MidiMessageRealTimeTimingClock());
msgCount=0;
}
}
else
{
if (MAX_MSG_COUNT > msgCount && 0 != next && _PreciseUtcNowTicks >= next)
{
next += dif / 24;
dev.Send(new MidiMessageRealTimeTimingClock());
++msgCount;
}
}
}
}
Real programmers use butterflies
modified 1-Jul-20 23:11pm.
|
|
|
|
|
The 'tight loop' has nothing to do with real time. Windows scheduler can interrupt your process at any time, as far as I know, so when it is interrupted your 'Console.KeyAvailable' will not be immediately called. A quick google brings up this page, with some info about the behind-the-scenes, which may or may not be appropriate.
So if I understand the problem, you are playing a composition through some app, and using this console app to inject timing messages into the playing composition? So you have device latency of the console app and latency of the playing app to take into consideration? (Or is latency overcame somehow through the 'MidiMessageRealTimeTimingClock' message (sending a precise tick?).
|
|
|
|
|
i didn't mean true realtime. I just meant realtime enough for MIDI. I know Windows is not a realtime OS.
And anyway, that app isn't the problem. Like I said, the timing of it is solid and I can verify that.
The other app - the receiver is the problem.
This is the routine. Note that the only code that executes on incoming time sync messages is the if block with this heading:
if (0!=_tempoSynchEnabled && 0xF8 == (0xFF & lparam))
That will evaluate to true when it's one of these messages and the rest of the code does not execute.
I cannot speed this up.
void _MidiInProc(IntPtr handle, int msg, int instance, int lparam, int wparam)
{
switch (msg)
{
case MIM_OPEN:
Opened?.Invoke(this, EventArgs.Empty);
break;
case MIM_CLOSE:
Closed?.Invoke(this, EventArgs.Empty);
break;
case MIM_DATA:
MidiMessage m;
if (0!=_tempoSynchEnabled && 0xF8 == (0xFF & lparam))
{
if (0 != _timingTimestamp)
{
var dif = (_PreciseUtcNowTicks - _timingTimestamp) * 24;
var tpm = TimeSpan.TicksPerMillisecond * 60000;
var newTempo = (tpm / (double)dif);
if (newTempo < _tempoSynchMininumTempo)
Interlocked.Exchange(ref _timingTimestamp, 0);
else
{
var timeNow = _PreciseUtcNowTicks;
if (0L==_tempoSyncTimestamp || 0L == _tempoSyncFrequency || (timeNow-_tempoSyncTimestamp>_tempoSyncFrequency))
{
var tmp = Tempo;
var ta = (tmp + newTempo) / 2;
Tempo = ta;
}
Interlocked.Exchange(ref _timingTimestamp, timeNow);
}
}
else
{
var timeNow = _PreciseUtcNowTicks;
Interlocked.Exchange(ref _timingTimestamp, timeNow);
}
}
else
{
m = MidiUtility.UnpackMessage(lparam);
_ProcessRecording(m);
Input?.Invoke(this, new MidiInputEventArgs(new TimeSpan(0, 0, 0, 0, wparam), m));
}
break;
case MIM_ERROR:
Error?.Invoke(this, new MidiInputEventArgs(new TimeSpan(0, 0, 0, 0, wparam), MidiUtility.UnpackMessage(lparam)));
break;
case MIM_LONGDATA:
case MIM_LONGERROR:
var hdr = (MIDIHDR)Marshal.PtrToStructure(new IntPtr(lparam), typeof(MIDIHDR));
if (0 == hdr.dwBytesRecorded)
return;
var status = Marshal.ReadByte(hdr.lpData, 0);
var payload = new byte[hdr.dwBytesRecorded - 1];
Marshal.Copy(new IntPtr((int)hdr.lpData + 1), payload, 0, payload.Length);
m = new MidiMessageSysex(payload);
var sz = Marshal.SizeOf(typeof(MIDIHDR));
_inHeader.dwBufferLength = _inHeader.dwBytesRecorded = 65536u;
_inHeader.lpData = _buffer;
_CheckOutResult(midiInPrepareHeader(_handle, ref _inHeader, sz));
_CheckOutResult(midiInAddBuffer(_handle, ref _inHeader, sz));
_ProcessRecording(m);
if (MIM_LONGDATA == msg)
Input?.Invoke(this, new MidiInputEventArgs(new TimeSpan(0, 0, 0, 0, wparam),m));
else
Error?.Invoke(this, new MidiInputEventArgs(new TimeSpan(0, 0, 0, 0, wparam),m));
break;
case MIM_MOREDATA:
break;
default:
break;
}
}
Real programmers use butterflies
|
|
|
|
|
I'm sorry. Your code is not as clean as you claim.
if (_tempoSynchEnabled && ((0xFF & lparam) == 0xF8)) {
Placing the result on the left is just ugly! And not using booleans in a boolean operation when you can???
Anyway, not being very familiar with your codebase, I'm assuming the _timingTimestamp is updated every played beat with the reference time. Or is it something triggered by the first console keydown message? Should be the first if you are changing tempo based on it. Then you need to have another structure for the first console keydown message. Or I am missing something. Hope this quick musing is helpful.
|
|
|
|
|
I can't use a boolean for that because interlocked exchange only works on 32-bit or 64-bit values.
That's documented where it's declared
And placing consts before vars is something you'll find among folks like me who cut their teeth on C and C++ when compilers didn't warn you about accidental assignments
_timingTimestamp holds the system ticks - basically the UTC "now" of the last timing message received. Every time, what we do is take the difference of this from the current time (also in ticks) and use that to get a period. We take the period - which is 1/24 of a quarter-note, and convert it to a tempo. We then use that tempo to update the incoming recording and such
Real programmers use butterflies
|
|
|
|
|
var dif = (_PreciseUtcNowTicks - _timingTimestamp) * 24;
var tpm = TimeSpan.TicksPerMillisecond * 60000;
var newTempo = (tpm / (double)dif);
I don't see any processing to take account of when the first keystroke from the console app reached this processing app. Yes I know about the old C coder's habit. Still ugly! Did not know about the interop though, can't you still treat it as a boolean, like you can in C++? A value of 0x0001 will still be 'true'.
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
