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

Brace 0.11.2 (bumps ace to 1.4.2; adds travis support) #152

Closed
wants to merge 6 commits into from
Closed

Brace 0.11.2 (bumps ace to 1.4.2; adds travis support) #152

wants to merge 6 commits into from

Conversation

StoneCypher
Copy link
Contributor

This PR:

  1. Bumps the underlying ACE implementation
  2. Adds support for Travis-CI assuming to check Node 8-11

@djMax
Copy link

djMax commented Jan 14, 2019

I would definitely love to see brace updated to the latest ace, it's tough to diagnose issues when the underlying dep is so far behind. react-ace is also suffering as a result and it'd be a lot easier (in theory) to fix their problem here than having them move off brace.

@StoneCypher
Copy link
Contributor Author

I pinged him on Twitter and he said he'd see to this mid-January

So I think this might be coming soon?

@StoneCypher
Copy link
Contributor Author

@thlorenz Hey buddy, could you please hit merge?

@thlorenz
Copy link
Owner

Hey looks good @StoneCypher , but I'd prefer two things:

  1. Don't bump the brace version .. I do this as part of the publish
  2. Provide the add travis part as a separate PR

So ideally this PR would have ONE commit which bumps the version in the update script and includes the result of running it (changed Ace files).

@StoneCypher
Copy link
Contributor Author

i don't understand what you mean by "bumps the version in the update script"

@thlorenz
Copy link
Owner

thlorenz commented Jan 15, 2019

I mean please don't change the brace version inside the package.json as I take care of that during the publish step to npm.
As in please remove this commit (along with the other ones unrelated to the upgrade).

@StoneCypher
Copy link
Contributor Author

I have removed the .travis.yml and regressed the version in package.json.

I believe that's what I'm supposed to do, but I have a bad flu and I'm stupid when I'm sick

@StoneCypher
Copy link
Contributor Author

I will reintegrate travis in a followup PR if you say this is what you meant

@thlorenz
Copy link
Owner

OK, could you please squash all this into one commit?
Basically all we need for this PR are the changes in this commit.

At this point there are tons of commits here including a merge of another PR which I'm not sure about why it is even here.
Alternatively open a new PR with just the upgrade related commit and we'll go from there.
Thanks.

@StoneCypher
Copy link
Contributor Author

Oh. Okay

@StoneCypher
Copy link
Contributor Author

ahaaaa haaaaaaaaaaaa downloading large repos on conference wifi

@StoneCypher
Copy link
Contributor Author

#153

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