search

labstreaminglayer / App-LabRecorder

An application for streaming one or more LSL streams to disk in XDF file format.
MIT License
124 stars 45 forks source link

Fix cfg parsing of StorageLocation #37

Closed cboulay closed 4 years ago

cboulay commented 4 years ago

Based on #36, Fixes #30

dmedine commented 4 years ago

Nice! I was going to mention the storage location bug. Thanks for including it.

cboulay commented 4 years ago

Line 141 is still wrong. https://github.com/labstreaminglayer/App-LabRecorder/pull/37/files#diff-585cfecf0ce15ac75be3038e77e65611R141 The right most part of the expression is evaluated first so the first becomes the second part of the string. Tristan and I had briefly discussed changing the syntax altogether so I never pulled in my bugfix for this (#24). I think we should, though, so at least it doesn't have a bug in it for the time being.

I'm not going to fix that in this PR. I'm working on something else in LabRecorder now and it'll be too confusing to fix that until a couple other things have been merged.

dmedine commented 4 years ago

It's literally 3 lines of code.

cboulay commented 4 years ago

But I've edited about 100 lines of code since then for the next thing I'm working on that's unrelated... but based off this PR. I don't want to hold this back because of the conversation that's inevitable on the next item.

dmedine commented 4 years ago

OK. No worries.

But, in that case, I would request holding off on a PR, until you are done with it. I don't want to go to the trouble of making reasonable suggestions if the reply is 'not yet, I'm still committing'. (No rudeness intended, if read that way! just trying to avoid miscommunications!)

The other option would be to agree to a policy that one should not comment on a PR unless review is requested specifically.

cboulay commented 4 years ago

It's not that this one isn't done, it is, I hope. The other PRs are different issues entirely. But they work on the same code base so I can't create independent PRs unless I want to deal with conflicts after each is merged.

dmedine commented 4 years ago

I don't know what you mean.

If you submit a PR from a branch and add a commit to that branch later, it automatically updates the PR.

If you branch off of that PR and make some changes and submit another PR to master, it is pretty hard to cause conflicts unless you are mangling the same sections you already put in the first PR, which you should not do anyway.

By the way, if you do piggy back your PRs like this (which I see that you have now done) the commit history from the first PR is included in the second one, which makes sense because it is the same branch. This PR will become irrelevant when #38 is merged. Github should close it automatically, in fact, unless you did the wrong thing in which case there would be a conflict in #38.

I just tested this with a toy repository.

dmedine commented 4 years ago

I believe that the 'right' way to do this is to create a new branch whenever you intend to do a PR, commit the changes and merge. If you want to work on multiple PRs at once, for some reason, then branch, commit repeat. If you do this hygienically there probably won't be any niggling conflicts when you submit the PRs and you can just merge the latest one and all the previous ones will fall into place.

An even better way to do it is for each developer to have his or her own branch and only ever commit to that one. Ideally there should be only one branch in the master repository and merges should be strictly controlled and piecemeal from individuals' branches when projects are completed, tested, and reviewed. But, this is also a lot harder because it requires code rebasing whenever you want to incorporate another developer's commits into your own branch---which is really hard and somewhat dangerous. It's also impossible to enforce this workflow with a public repo.

cboulay commented 4 years ago

I feel like these discussions can go in #38 which I fully expect to get held up in conversation.

Let’s please keep on topic for this one.

dmedine commented 4 years ago

My comments have all been on topic. The change I requested related exactly to the title of the PR until you changed its name!

Sorry to be prickly, but that was a little shady.

Also, I am sorry to have to say this but if you want to go stacking PRs like this, you need to accept the fact that it is indeed confusing; or, you need to accept the comments from other stakeholders who point out that for that very reason, doing so is not line with best development cycle practices.

I will fix the post_sync options on a separate PR, so don't worry about it.

Since you stacked 38 on top of this, and refuse to have it reviewed on its own, it has become irrelevant and I will close this discussion.

cboulay commented 4 years ago

I changed this PR's base to datetime (in #36) to isolate the code changes that I hope to discuss in this PR.

dmedine commented 4 years ago

I am sorry that I closed this. All I ever wanted was to fix the config file parsing bugs which I thought was what this was about. I won't comment further because I have nothing to say about the datetime issue.