-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Passive Engine Re-write for Paralellism #51
Conversation
- wip: passive engine rewrite - wip: general code refactor - wip: some missing code (pick from master)
I think it would be good to make these kinds of changes in multiple commits. There's some additional refactoring I can see here but given that it's all in one commit it's a bit tricky for me to test and track the changes. I'll start that work once this is merged though (I'd already started some of it already when adding more verbose error handling, but I'll come back to it in this branch). The code itself looks good, is there something further you're working on before merging with this? |
@Mzack9999 as codingo noted, there is some scope for more refactoring here, changing the naming scheme, making some small changes to make it compatible with the latest commits i made. But, you have done an amazing work. Once you are finished, notify us and we will tidy up things a little bit. |
86f5dfc
to
254d501
Compare
… passive-engine-rewrite
I think I'm done with the big changes. Imho next PRs should be about:
|
@codingo Probably you can review it. You also wanted to do some more refactor, right? |
I do, but I think it would be best in a separate branch so we can put a release tag on this to keep everything clean. I'll set this up in the test bench shortly! |
@Ice3man543 looks to satisfy testing on my end. Since it's a rather significant change suggest you do the same and then we can merge. |
Sure @codingo |
@codingo This doesn't complies with the new output style we created in ui rewrite branch. Also, there are a few things that I don't understand. Probably need to refactor some more and make this a bit more easier to understand. All in all, it's great work by @Mzack9999 👍 |
@Ice3man543 let me know if you need any help |
@Mzack9999 So i finally took a look at the code and ran it. The tool has became a lot more faster 😄 thanks to you. So great work 👍 Keep it up. But there are some issues with the pull request that stop me from merging it right now. It's a lot more faster though :).
If all these mistakes can be fixed by you, then awesome. Otherwise, i am going out of town for a few days. After i return, i can fix these and merge the branch. |
…sed later by aquatone writer
I replaced helper.Job with a more generic helper.Domain. I'll try to correct in next days the various outputs |
@Ice3man543 @codingo hopefully I merged correctly the output styles from master, I just left a TODO related to output to directory which I've no time to address right now, but should be a fast fix |
@Mzack9999 I am working on fixing some things. Will fix everything i can see and then merge :) |
@Mzack9999 The aquatone style output is still messed up. I am working now on cleaning it. Will be done by morning i guess if i leave everything and work on this! Do you think merging it to the master is correct? Also, have you done any benchmarks? I will do them :) |
Oh god, i merged it by mistake :( |
@Mzack9999 The aquatone -nW flag output doesn't works as expected. Check it out. |
@Mzack9999 @codingo No worries now. I made some chages so that it works fine. Now, everything is working as supposed and even faster due to Worker pools and create a more modular architecture :) |
@codingo @Ice3man543
Changelog