Much simpler route to upgrading 1.4.2

thlorenz / brace

📔 browserify compatible version of the ace editor.

http://thlorenz.github.io/brace/

MIT License

1.06k stars 304 forks source link

Much simpler route to upgrading 1.4.2 #153

Open StoneCypher opened 5 years ago

StoneCypher commented 5 years ago

Overdoing it less this time, now that I know better

StoneCypher commented 5 years ago

Also I'll remake the travis PR immediately after you tell me if this is what was needed, @thlorenz

thlorenz commented 5 years ago

Please run the update script as well so the ace files actually update. Once that's done run npm test to verify that all annotations are still present.

Then I can do the squashing and will publish a new version.

StoneCypher commented 5 years ago

Okay. Sorry about misunderstanding your instructions repeatedly. I'm very stupid when sick.

This version of this PR:

Contains the one-liner change
Contains the post-build code
Has had in-browser tests run
Does not contain the .travis.yml
Does not contain the instructions I'm about to write, so that you don't have problems with people like me in the future 😀

thlorenz commented 5 years ago

OK, thanks I'm busy today but will merge and publish tomorrow granted all goes well. Thanks for your patience :)

StoneCypher commented 5 years ago

Thank you for an excellent tool, and putting up with my getting it wrong so many times

Now that I know what I'm doing, I'm likely to submit more upgrades in the future, if that's acceptable to you.

thlorenz commented 5 years ago

OK I tried to apply your change, but after removing all the white-space only changes, only the version (index.js) and the types (index.d.ts) where updated.

So I reran the update script and got the same result:

➝  git diff -w --stat
 ext/emmet.js             |   0
 ext/language_tools.js    |   0
 ext/searchbox.js         |   0
 index.d.ts               | 157 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------
 index.js                 |   2 +-
 keybinding/emacs.js      |   0
 keybinding/vim.js        |   0
 mode/bro.js              |   0

[ many more files with 0 changes that aren't white space only ]

I applied this to master but didn't publish for now. Could you please verify that this indeed includes the changes that should come with the new version. Maybe you need to check in with the Ace devs to see what else should have changed that we may have missed (it could be that the update script we use is out of date).

If all checks out I'll publish this as a new minor.

thlorenz commented 5 years ago

Oh, also I removed the package-lock from the PR .. please don't add that in the future.

thlorenz commented 5 years ago

Since this is actually so sketchy I removed this commit from master and pushed to a separate branch. I don't know what's going on, but all I can see now is the index.d.ts file changed.

Could you please do another upgrade, remove all white-space only changes so we can verify in a PR what actually DID change? It is really hard to tell with all the white-space noise. But I'm pretty sure something isn't right here.

Thanks.

thlorenz commented 5 years ago

I noticed that somehow a version change to index.js is getting lost after I applied your PR and removed the whitespace stuff. But again that's the only thing that really changed (that can't be right).

The whitespace changes are on the Ace team actually (not your doing), but again it's really hard to see what's going on, so ideally remove them before submitting this as a PR.

tankbusta commented 4 years ago

@thlorenz anything we can do to get this merged in?