PrismarineJS / prismarine-provider-anvil

Anvil Storage Provider implementation.
13 stars 26 forks source link

Support all latest versions, add even more tests for other issues #79

Open zardoy opened 3 months ago

zardoy commented 3 months ago

re opened pr as changed the branch

socket-security[bot] commented 3 months ago

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/prismarine-chunk@1.35.0)

View full report↗︎

extremeheat commented 3 months ago

Instead of closing and reopening an identical PR you should instead either do a force push where needed to change history or commits. This prevents the PR from needing to be re-reviewed from scratch.

zardoy commented 3 months ago

Instead of closing and reopening an identical PR you should instead either do a force push where needed to change history or commits. This prevents the PR from needing to be re-reviewed from scratch.

AFAIK, you cannot change the source branch after creating a Pull Request. You have to create a new one instead!

zardoy commented 3 months ago

all comments from the previous pr are resolved, now, I only need to think about what to do with failing tests: should I remove them or disable them somehow for now?

extremeheat commented 3 months ago

AFAIK, you cannot change the source branch after creating a Pull Request. You have to create a new one instead!

You cannot change your branch, but you can change the commits on the branch. If you need to change the commits on the branch and change history git makes it possible to update the history with a force push. If you need to make a new branch locally and push to a remote branch with your PR you do git push origin localBranch:remoteBranch --force.

all comments from the previous pr are resolved, now, I only need to think about what to do with failing tests: should I remove them or disable them somehow for now?

They are not resolved

zardoy commented 3 months ago

You cannot change your branch, but you can change the commits on the branch. If you need to change the commits on the branch and change history git makes it possible to update the history with a force push. If you need to make a new branch locally and push to a remote branch with your PR you do git push origin localBranch:remoteBranch --force.

Yes, the previous source branch is the default branch of my repo and I pushed some experimental unrelated to PR changes for my projects. In theory, I could rename it to something else, create a new branch, and set it as a new default branch with the old name. Sorry, I forgot there is a simple way to rename remote branches.

They are not resolved

Can you elaborate? I don't see anything that I made worse..

zardoy commented 3 months ago

tests are green, I will be waiting for your reply

extremeheat commented 3 months ago

Yes, the previous source branch is the default branch of my repo and I pushed some experimental unrelated to PR changes for my projects. In theory, I could rename it to something else, create a new branch, and set it as a new default branch with the old name. Sorry, I forgot there is a simple way to rename remote branches.

No, "renaming" the branch will either auto close the PR or move the PR's source branch with it. The only way to fix it is to keep the current remote and push a new history to it. I don't see anything different in this PR to the last one, but I've re-reviewed it.

zardoy commented 3 months ago

I don't see anything different in this PR to the last one, but I've re-reviewed it.

I already pointed that I couldn't keep the same remote, it was not because of different history. At least I made tests green in this PR. And yes, not all was tested on master (and I increased the number of tested versions in writeNBT), that's why there are some of the changes to fix them

rom1504 commented 3 months ago

until https://github.com/PrismarineJS/prismarine-provider-anvil/issues/75 if fixed, it won't be possible to support newer versions

zardoy commented 3 months ago

until #75 if fixed, it won't be possible to support newer versions

how is this related? new versions almost don't have any changes and will work with 1.18 implementation fine (just like in p-chunk)

rom1504 commented 1 week ago

please address comments or close the PR

zardoy commented 1 week ago

please address comments or close the PR

easy to say...