PartKeepr
PartKeepr copied to clipboard
partkeepr
Merge Bug Fixes
This branch contains the following bug fixes;
fixes: #997, #989, #418, #1002
It also fixes exporting issues, allows the root category to not be set as default, fixes prices in stoc history, and fixes a rounding issues in parameter searching.
hey @vmw I'm going to merge my ci/cd fixes soon (want to make sure I don't change anything important) and then lets see how your branch builds against develop
Although it would've been easier to review if the bugs where treated in their own PR
Ok yeah you're going to need to break this up in to more manageable chunks. At this moment this PR is impossible to review as it contains too many unrelated commits.
I saw a couple of very useful fixes, but each really deserves their own PR that can individually be reviewed and merged.
@vmw Vincent - could you create individual PRs? it will enable the merge far easier and also allow us to note which bug each one fixes... sorry for the inconvenience, but sometimes merging PR can be really hard..
Yes: I'll look into breaking them up to make it easier. Unfortunately, we forked this build, and, as you previously noted, split up some of the commits to make it easier for ourselves (internally).
I'll see whether I can cherry-pick some from master and I'll list them as separate requests. I fully expect that some of them will (should?) be rejected as we opted to deliberated work around some areas of the code that were causing us issues.
This might take a bit of time because, for some of the commits, we included temporary files which should be cleaned up. I'm looking into this now.
Cool, looking forward to see some of the bug-fixes.
I want to get this one travis-ci bug fixed though, before we start merging new code. (any help in that branch is appreciated)
I've created separate bug fixes, but it's going to fail the style guide. I had previously fixed that manually, but you if you accept the previous PRs, you should be able to easily use this commit to clean up the code, as it's all in a single commit (the last two).
Thnx for opening these. It would be better if the merged code is already fixed for style of course, the whole point of fixing the styleci is to help with merging new code.
Best would be to rebase your code to the current master to pull in all configuration and style changes, so we can see the working builds coming in (lets fix travis-ci first though).
@dromer I realize the style CI should have been implemented within each commit. Unfortunately, we don't immediately have the engineering resources to review each commit - I did try and break up each commit as best I could manually, but I don't think I can polish each one to the level they should be.
The problem is that we do development locally, so we don't realize the style guide issues until after we push the fixes. At the time this was done, we didn't realize it was a problem until the last minute, so I'm the one who came in to fix it one-by-one.
I want to get this one travis-ci bug fixed though, before we start merging new code. (any help in that branch is appreciated)
@dromer See #1070.
The problem is that we do development locally, so we don't realize the style guide issues until after we push the fixes. At the time this was done, we didn't realize it was a problem until the last minute, so I'm the one who came in to fix it one-by-one.
Yes I know, that's what I don't like about styleci, there's no way to easily run it locally for review (we use flake8 at work saves a lot of headaches).
That's why you could merge master branch back into your pr-branches and pull in the fixes that way. Anyway the tests are more important than the style.
Ok, current master should have working builds (style and unit tests) again.
Please if you can do a git merge origin/master <your-fix-branch> from each branch and update your PRs.
I don't immediately have the bandwidth to do this - it will likely take me a several weeks to get to this. We're using our forked version, so I'm happy to close the PRs if you'd prefer that instead and manage this downstream for the time being?
Sorry, I was unclear:
We don't mind hosting a downstream version of partkeepr and simply cherry-picking or patching the upstream version. The problem is that we don't immediately have the bandwidth to do the complete code review - the engineer working on this isn't here anymore. As a result, it falls on me, and I don't have the bandwidth to properly format the code.
We know it works, and compiles, on our current installation, and this (mega) commit also had the style guide fixes applied on top of all the bug fixes. I definitely realize that this is far from the best PR, and also note that the bug fixes aren't atomic. I did try and clean that up, at least, in the individual PRs.
However, because the entire style guide review process requires the git website, it's a lot more work to go back and individually fix the separate PRs for style. (This was actually the most time consuming part of the process - if you have a command line linter or something I could run locally - similar to flake8 or lint - it would speed things up).
I definitely understand that this is undesirable grunt work, so I'm just suggesting that I close them and I maintain them locally until we have the bandwidth to fix them.
I agree that it is quite annoying that you can't run StyleCI locally, but this is what we have at the moment. (I don't know a decent equivalent for PHP like flake8)
Would something like this not work for styling php in PartKeepr?
https://mike42.me/blog/2018-06-how-to-check-php-code-style-in-a-continuous-integration-environment
John Pateman
Sent from my iPhone
On 19 Feb 2020, at 07:31, dromer [email protected] wrote:
I agree that it is quite annoying that you can't run StyleCI locally, but this is what we have at the moment. (I don't know a decent equivalent for PHP like flake8)
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.
I would consider PHP_CodeSniffer a useful addition. @Gasman2014 did you use it yet? Is StyleCI configured to be running on any of the provided styles that PHP_CodeSniffer provides?
Adding it to travis should be no big problem.
I have no experience of using this (I mainly use python / c ) but, if it does what it says on the tin, it might go some way towards mitigating your code formatting issues.
John Pateman
Sent from my iPhone
On 19 Feb 2020, at 09:46, Christian [email protected] wrote:
I would consider PHP_CodeSniffer a useful addition. @Gasman2014 did you use it yet? Is StyleCI configured to be running on any of the provided styles that PHP_CodeSniffer provides?
Adding it to travis should be no big problem.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.
Is it possible to re-run the travis-ci tests? I recall that there may have been issues with the CI system, and I'm interested in seeing whether it now passes all of them.
I've also updated the conflicts against this branch, if you'd also like to take a look at this one.
It would be much better if each of these fixes come in as separate PRs.
Further more I see a lot of small issues with this PR. Random things commented out or unused things brought in.
As is I don't see why this would be merged.