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

Makefile colorizing improvement: colorize at the beginning of line, handling the '\' symbol and other. #27293

Merged
merged 3 commits into from Aug 21, 2017

Conversation

fadeevab
Copy link
Contributor

@fadeevab fadeevab commented May 25, 2017

Pull request is created into TextMate repository as well: textmate/make.tmbundle#15

1). Colorize when there is an expression at the beginning of line.
2). Handle '\' at the end of prerequisites (the next line should be colorized as a continuation of previous).
3). Colorize (origin|flavor).
4). Fix colorizing of built-in which stands after a common variable.

makefile_before
makefile_after

… 2). Handle '\' at the end of prerequisites. 3). Colorize (origin|flavor). 4). Fix colorizing of built-in standed after a common variable.
@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@aeschli
Copy link
Contributor

aeschli commented May 25, 2017

@fadeevab Sorry for my previous comment, now I see your pull request.
Give it a bit time for the guys at textmate/make.tmbundle to look into it.
If nothing happens there we can think again what to do and if we make an exception and move ahead.

@aeschli aeschli reopened this May 25, 2017
@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@fadeevab
Copy link
Contributor Author

@aeschli, Sure, thank you! It would be great if somebody review my pull request more carefully. But actually, current makefile colorizing couldn't be worse, so I believe this patch makes one better. At least I tried to test it carefully, that's why I recreated pull request with additional fixes. And also several fixes will come later, because some cases are still not covered.

@fadeevab fadeevab closed this May 26, 2017
@fadeevab fadeevab reopened this May 26, 2017
@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@fadeevab
Copy link
Contributor Author

(re-opened because it's closed accidentally from mobile phone)

@fadeevab
Copy link
Contributor Author

Nothing happens in the referred pull request. Wait more?

@fadeevab
Copy link
Contributor Author

fadeevab commented Jun 2, 2017

  1. There is some movement in the referred review at TextMate Makefile colorizing improvement: colorize at the beginning of line, handling the '\' symbol and other. textmate/make.tmbundle#15.
  2. I actualized makefile tests.
    makefile_test_fixture

@fadeevab
Copy link
Contributor Author

fadeevab commented Jun 2, 2017

A moment ago both builds were ok. Now AppVeyor build failed, I'm sure it doesn't depend on update.

npm ERR! code EBADF
npm ERR! errno -4083
npm ERR! syscall fstat
npm ERR! EBADF: bad file descriptor, fstat

@fadeevab
Copy link
Contributor Author

Review is stuck

@fadeevab
Copy link
Contributor Author

@aeschli, TextMate review of Makefile syntax is not moving on. What could we do in such case: could we submit?

@aeschli
Copy link
Contributor

aeschli commented Jun 12, 2017

Let's be patient one more week or so. Creating our own fork is not really what we want as we don't have the resources to maintain it. So given that you got some response on your PR I'd rather wait a bit more.

@fadeevab
Copy link
Contributor Author

Right, I just don't want this patch would be sank into oblivion.

@fadeevab
Copy link
Contributor Author

@aeschli, 3 months are passed. I give up to push TextMate team. You can see there is a lot of my comments, and also I divided my commit into 4 commits there. There is only one reviewer and he doesn't take an action.

@aeschli
Copy link
Contributor

aeschli commented Aug 16, 2017

What we can do is that we use your fork (https://github.com/fadeevab/make.tmbundle/) as the new input for the make grammar. Would that be ok for you?

  • you'd have to make sure to keep master in a working state
  • avoid diverging from the original so that we eventually can unify them again.
  • new issues would be filed against your repo

Ideally this is only a temporary solution.

Let me know when your repo is ready.

@fadeevab
Copy link
Contributor Author

Just let's do this. I agree to send new pull request to the original repo to synchronize our repos, so we can eventually unify them. Also I'll try to take care about new issues, still maybe it worth to duplicate the issues to the original repo.

@fadeevab
Copy link
Contributor Author

@aeschli, Any chances? :)

@aeschli aeschli merged commit 7f19126 into microsoft:master Aug 21, 2017
@aeschli
Copy link
Contributor

aeschli commented Aug 21, 2017

Thanks @fadeevab !

@aeschli aeschli added this to the August 2017 milestone Aug 21, 2017
@aeschli aeschli added languages-basic Basic language support issues feature-request Request for new features or functionality labels Aug 21, 2017
@aeschli
Copy link
Contributor

aeschli commented Aug 21, 2017

@fadeevab I'm now consuming your repo. To simplify things, can you add an MIT LICENSE.md file to your repo?
I'd suggest to append the licence mentioned in https://github.com/fadeevab/make.tmbundle/blob/master/README.mdown to that

@fadeevab
Copy link
Contributor Author

@aeschli, Done

@aeschli aeschli added the verification-needed Verification of issue is requested label Aug 30, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality languages-basic Basic language support issues verification-needed Verification of issue is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants