Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[LevelDbStore] Updated features & Usability of Interop #3274

Closed
wants to merge 17 commits into from
Closed

[LevelDbStore] Updated features & Usability of Interop #3274

wants to merge 17 commits into from

Conversation

cschuchardt88
Copy link
Member

Change Log

  • Enabled all the features of leveldb
  • New Classes, Memory management and interoperability.

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@Jim8y
Copy link
Contributor

Jim8y commented May 24, 2024

change like this better split into multiple small tasks and start working on a specific branch.

@cschuchardt88
Copy link
Member Author

Why is so hard for you review a PR? No more than 10 lines, everyone throws a fit. It an application as a whole. No one is going to approve a PR if it doesn't work or incomplete application. It like me making a PR for a HelloWorld console app. Even if it was on another branch it not going to get approve until years and could even be forgot. Do it right the 1st time and I wouldn't need to clean up. There is no thought put into code design. Not my fault and everyone wonders why no one uses it. So stop saying small PR. Small PRs are for bug fixes. How are we to expand? Plus I am moving this from neo-modules. My code is straight forward and easy to follow. Sorry for out burst but its at everyone not you @Jim8y.

@cschuchardt88 cschuchardt88 added Waiting for Review Enhancement Type - Changes that may affect performance, usability or add new features to existing modules. Optimization Optmization issues labels May 25, 2024
@Jim8y
Copy link
Contributor

Jim8y commented May 25, 2024

i have being following with this issue for a while, and have discussed with multiple coredevs, turns out everyone is actually working hard and busy reviewing. the real problem i believe is the coredev system itself, reviewers are not responsible for reviewing a pr, and big prs usually will potentially have more issues and need more time to check, but with the same amount of time, we can revoew many other smaller prs. its a system issue, not a single one of us can change it, but adapt it.

@Jim8y
Copy link
Contributor

Jim8y commented May 25, 2024

so i highly recommand you to split it. or i can do it for you without taking your bonus.

shargon
shargon reviewed May 26, 2024
View reviewed changes
src/Plugins/LevelDBStore/Result.cs Outdated Show resolved Hide resolved
src/Plugins/LevelDBStore/SnapShot.cs Outdated Show resolved Hide resolved
src/Plugins/LevelDBStore/SnapShot.cs Outdated Show resolved Hide resolved
@cschuchardt88
Copy link
Member Author

cschuchardt88 commented May 26, 2024 •
edited
Loading

I could see this being a problem. If we have deadlines. But we don't. Its only 32 files, many small. It should be the job of the tester, to test it and use it for some time, to make sure its in working order. Also the job of the developer doing the PR to do the testing. Most PR's are un-tested before merge. So I can understand your point of view. Just know this I test and test my code before I make PR. I am over compulsive about style, code functionality and code design.

@Jim8y Jim8y requested a review from a team May 27, 2024 23:45
shargon
shargon reviewed May 28, 2024
View reviewed changes
src/Plugins/LevelDBStore/Logger.cs Outdated Show resolved Hide resolved
src/Plugins/LevelDBStore/Options.cs Show resolved Hide resolved
src/Plugins/LevelDBStore/Options.cs Show resolved Hide resolved
@Jim8y Jim8y requested a review from a team May 28, 2024 08:08
@lingyido
Copy link
Contributor

Please take all your time for this one. I see no good reason for updating new features for levelDB which is an essential modules. I personally disagree these great changes.

@cschuchardt88
Copy link
Member Author

Please take all your time for this one. I see no good reason for updating new features for levelDB which is an essential modules. I personally disagree these great changes.

Its more stable and done properly in dotnet for the GC and other things.

@cschuchardt88 cschuchardt88 mentioned this pull request Jul 2, 2024
Closed
@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Jul 4, 2024 •
edited
Loading

No Performance Increase, Stayed the same

image

@cschuchardt88 cschuchardt88 requested a review from Jim8y July 5, 2024 03:48
@cschuchardt88
Copy link
Member Author

Check Simplified PR #3414 with minimal changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shargon shargon shargon left review comments

@Jim8y Jim8y Awaiting requested review from Jim8y

Assignees
No one assigned
Labels
Enhancement Type - Changes that may affect performance, usability or add new features to existing modules. Optimization Optmization issues Waiting for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cschuchardt88 @Jim8y @lingyido @shargon