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

Much simpler route to upgrading 1.4.2 #153

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

StoneCypher
Copy link
Contributor

Overdoing it less this time, now that I know better

@StoneCypher
Copy link
Contributor Author

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

@thlorenz
Copy link
Owner

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
Copy link
Contributor Author

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

This version of this PR:

  1. Contains the one-liner change
  2. Contains the post-build code
  3. Has had in-browser tests run
  4. Does not contain the .travis.yml
  5. 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
Copy link
Owner

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

@StoneCypher
Copy link
Contributor Author

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
Copy link
Owner

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 pushed a commit that referenced this pull request Jan 19, 2019
- NOTE: only types and version were changed with this update (index.js
and index.d.ts)
  - all other files had white-space only changes

PR: #153
thlorenz pushed a commit that referenced this pull request Jan 19, 2019
- NOTE: only types and version were changed with this update (index.js
and index.d.ts)
  - all other files had white-space only changes

PR: #153
@thlorenz
Copy link
Owner

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

@thlorenz
Copy link
Owner

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
Copy link
Owner

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants