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.

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

Remove StatesDumper - Minor Refactor on StorageDumper #3281

Merged
merged 5 commits into from
May 28, 2024

Conversation

vncoelho
Copy link
Member

close #3271

@vncoelho vncoelho requested review from cschuchardt88, roman-khimov and superboyiii and removed request for cschuchardt88 May 27, 2024 11:20
@vncoelho
Copy link
Member Author

@superboyiii @roman-khimov, I suspect that you are the ones that use this plugin.
I have been using StateDumper, but as I verified, StorageDumper was updated by @Ashuaidehao and it looks like you both are using StorageDumper.

In this case, I deleted StateDumper.

@Jim8y
Copy link
Contributor

Jim8y commented May 27, 2024

@superboyiii @roman-khimov, I suspect that you are the ones that use this plugin. I have been using StateDumper, but as I verified, StorageDumper was updated by @Ashuaidehao and it looks like you both are using StorageDumper.

In this case, I deleted StateDumper.

My suggestion is DO NOT DELETE IT DIRECTLT, but mark it as [Obsolete], no matter there is someone using it or not, we should not just delete an existing thing without telling the eco-system and warn them.

@vncoelho
Copy link
Member Author

vncoelho commented May 27, 2024

@superboyiii @roman-khimov, I suspect that you are the ones that use this plugin. I have been using StateDumper, but as I verified, StorageDumper was updated by @Ashuaidehao and it looks like you both are using StorageDumper.
In this case, I deleted StateDumper.

My suggestion is DO NOT DELETE IT DIRECTLT, but mark it as [Obsolete], no matter there is someone using it or not, we should not just delete an existing thing without telling the eco-system and warn them.

Both plugins are the same @Jim8y .
There is no sense the leave DUPLICATED code here in the core.

@vncoelho
Copy link
Member Author

I updated my last comment because I thought the message was from @superboyiii...aehuahea

There is no sense in what you said @Jim8y, you should check the code first.
As I said above.

@Jim8y
Copy link
Contributor

Jim8y commented May 27, 2024

I updated my last comment because I thought the message was from @superboyiii...aehuahea

There is no sense in what you said @Jim8y, you should check the code first. As I said above.

I am not saying you should keep duplicate code, what I mean is you should not delete existing thing directly. Its duplicated, but its also a neo product, a product should not be removed all of a sudden, but give a warning first, then remove later.

@vncoelho
Copy link
Member Author

I updated my last comment because I thought the message was from @superboyiii...aehuahea
There is no sense in what you said @Jim8y, you should check the code first. As I said above.

I am not saying you should keep duplicate code, what I mean is you should not delete existing thing directly. Its duplicated, but its also a neo product, a product should not be removed all of a sudden, but give a warning first, then remove later.

I understand your point of view. However, this is not the case now @Jim8y.

Furthermore, it is COMPLETELLY SIMPLE TO MODIFY to the other plugin instead, if someone is currently using the StateDumper.
However, I doubt someone is using it because perhaps StorageDumper fixed a minor problem that StateDumper had in some edge cases.

@Jim8y
Copy link
Contributor

Jim8y commented May 27, 2024

You can still do the same, i just suggest to do it latter, after one release. But i am ok if other coredevs think its fine.

@Jim8y Jim8y requested a review from a team May 27, 2024 23:40
@Jim8y
Copy link
Contributor

Jim8y commented May 27, 2024

I would suggest you write a readme to introduce the plugin.

@vncoelho
Copy link
Member Author

I would suggest you write a readme to introduce the plugin.

That a good idea. I will check how current Readme is.

@roman-khimov
Copy link
Contributor

If they have the same functionally and one can be replaced with another then deletion is fine and it'd just be a release note --- "if you're using X, please use Y now". And it's not a very popular plugin, not RpcServer for sure.

We have a SaveStorageBatch setting in NeoGo (https://github.com/nspcc-dev/neo-go/blob/master/docs/node-configuration.md) that exports the same data and then we have https://github.com/nspcc-dev/neo-go/blob/master/scripts/compare-dumps/compare-dumps.go to compare dumps which we do from time to time (I hope we'll be doing it less often after #1526). From what I see here, the dump format stays the same, so it's perfectly fine for us.

@shargon shargon merged commit df06791 into master May 28, 2024
6 checks passed
@shargon shargon deleted the organize-plugins-statesStorageDumper branch May 28, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove ONE OF THE duplicated PLUGINS StatesDumper and StorageDumper
4 participants