Cleaning up pull requests - Reviewed minor fixes and enhancements #1067
Comments
|
@dblessing Any news on this? |
|
No commits since october for the project - it this unmaintained? |
|
👆 Same, would like to know if this project is still considered active or not |
|
I started pulling in PRs into a new branch of Rouge before coming across this PR and the great work @vidarh had already done. I intend to go through the PRs reviewed here and merge them in. I use Rouge for my blog and would like to see it being maintained again. An actual fork seems less than ideal; I'm hopeful that if I can create a more up to date branch it can be merged back into this repo in due course. I don't have much experience with this kind of situation and don't know what the best way to proceed is. Any thoughts? |
|
With a couple of exceptions (mostly to do with missing file extension tests in the relevant spec), all the PRs identified above have been merged in. The other ones have PRs open with comments from me to the original authors requesting the relevant fixes. |
|
I do remember that GitLab was committed to maintaining this library (by way of @dblessing), along with @gfx. I do not know what happened to that arrangement. |
|
cc @stanhu |
|
@jneen I've exchanged a few e-mails with @dblessing about it; that was actually what prompted me to go through and review and pull together this list and the other linked issues, but I believe he's also been extremely busy (I did get an e-mail in March telling me he'd not forgotten about it). I'd be happy to spend more time reviewing outstanding changesets etc. if we have a way of actually getting them merged. I think really the project needs a few more approved committers, and offered myself up to @dblessing as he seemed to be more recently active than you, and he proposed I go through and review things with a view to demonstrating I'd do it seriously before verifying with you if you'd be ok with it, as I realise it's a big leap of trust to give people write access. If you have enough time available to have a discussion about that now, then great [I also really don't care who is able to do this, as long as someone is - I offered to help out mostly out of frustration..]. If you don't have time, I'll nudge Drew again. I'm also happy to go over @pyrmont 's PR and check it's all ok if that makes you or Drew more prepared to merge it. (thanks for nudging people to move this forward @pyrmont ..) |
|
Given that Rouge is now the preferred/recommended syntax highlighter in Asciidoctor, the Asciidoctor project is willing to help keep an eye on the repository and review/merge PRs when needed. (In fact, we have several PRs outstanding). I would be happy to be that delegate, though perhaps we could have at least one other member of the Asciidoctor team designated as a committer as well. I'll post back when I have a name. |
|
I appreciate the show of support! As I am currently writing my graduation thesis, I don't have a lot of time to devote, so I defer to the judgment of @dblessing and @gfx - if one of them would not mind passing along the direction and intentions of the project, I would be really happy. It's important with a project as large as this that new contributors do not take the shortcut of merging any pull request that comes along, as many will contain large amounts of copy-pasta and non-ruby-ish code (many contributors do not use ruby as their main language). |
|
@jneen Thanks for following up. Rest assured, I'd never merge a PR without testing it extensively (in fact, I probably go overboard with the review process, as my reputation demonstrates). But even then, requiring good tests is really the key to maintaining quality because there's only so much we can check by sight alone. |
|
Agreed! However - and this is important - unit-testing lexers is a well-known dead-end, which is why we have visual specs. The problem is that unit tests that expect an exact token stream as a result are incredibly brittle, and will break in difficult-to-fix ways the moment we incorporate a slightly different lexing strategy or a language version update, for example. |
|
Also, due to the nature of programming languages and the explosion of cases, it is much easier to test in a way that gives immediate feedback. And we don't lose much because we can guarantee that the lexers are independent of one another. |
|
Keep in mind with what I'm about to say that I don't maintain a syntax highlighting library, so I could be over my head here, but here goes... If the concern is that the syntax highlighting would change unexpectedly, I would prefer those brittle tests over visual ones. It doesn't take that long to fix assertions in my experience. And if the assertions are breaking, I'd want to know why and be able to completely understand the change. Those brittle tests are, in fact, precise tests (/me ducks) |
Right... if you add brittle tests, you will effectively block any possible future refactor of the code or the highlighting system. We do not, and should not, consider a change from Please do not go through every lexer and paste in a list of all tokens generated by the examples in the name of test coverage >< |
|
(we do also spec that the concatentation of all tokens equals the source input, to make sure that we don't drop any text - i think... if not then we should, because that's a universal invariant) |
|
@jneen regarding "we do also spec that the concatentation of all tokens equals the source input, to make sure that we don't drop any text - i think... if not then we should, because that's a universal invariant". When I wrote my lexer I realized that there was no way for me to know if my lexer was dropping any characters... that's why I submitted #852 |
|
Thanks to @pyrmont for telling me the situation. I'll see PRs listed above, but I think it's the best if @jneen create a new GitHub org and add someones to admins/owners/collaborators. Maintaining Rouge is somewhat difficult because reviewers have to decide OK or not on unfamiliar languages, so I think we need more maintainers. |
|
I'm all for adding new maintainers to help out. I'm still working on getting more official assistance from GitLab but that process is moving slowly. As we add maintainers I still think we need to be 'picky' and vet them, as via a process similar to this where someone reviews PRs for a while and pings existing maintainers for final approval. Once things look good then we can add as a maintainer. |
|
@gfx welcome to rouge-ruby/rouge! |
|
Thanks @jneen ! |
|
Long live rouge-ruby/rouge!! Thanks @jneen! Thank you for extending the life of this fantastic project! |
|
@vidarh Thanks again for this list. I hope you don't mind but I edited the OP to cross off the items that have been closed. I'll try to keep it up to date as we progress through the backlog :) |
|
@pyrmont Of course - great to see things getting merged. Might have to take a stab at some of the remaining bugs now that things are happening.. |
|
This issue has been automatically marked as stale because it has not had any activity for more than a year. It will be closed if no additional activity occurs within the next 14 days. |
@dblessing - as per e-mail here is a list of pull requests for bug fixes and enhancements I have reviewed that I consider fit to merge as is. As for #1063: I'm happy to create a single pull-request with squashed commits, one for each of these pull requests if you're ok with this list and if it'll help get them merged. As for #1063, sorting #1062 is a pre-requisite so we have CI working again.
Bug fixes
Fix XML not being detected as XML because of "<html" #1031 Fix XML not being detected as XML because the file contains <html somewhereimplemented Matlab2017a strings, fix #1047 #1048 Implement Matlab2017a strings; fix Matlab R2017a strings using double quotes #1047Restore support for highlighting XML-encoded plists #1026 Restore support for highlighting XML-encoded plistssedaddresses that use custom delimiter. #893 sed: Fix issue Handling sed format may hide some text #860 - handling sed format may hide some text.Enhancements
Update Scala lexer to output more differentiated token types. #1040 Update Scala lexer with more differentiated tokensKotlin suspend keyword #1055 Kotlin Suspend keywordAdd missing CoffeeScript keywords and reserved words #1061 Add missing CoffeeScript keywords and reserved wordsAdded missing tokens from Pygments 2.2.0 #1034 Add missing tokens from Pygments 2.2.0My own fixes
I consider these ok, but I'm the only one who has reviewed them:
Fix #1022 by supporting diffs other than unified. #1068 Fix Diff syntax highlight doesn't handle all cases #1022 (diff syntax highlighting for non-unified diffs) and close Fix highlighting of diff file/date lines #846 by adding highlighting of diff headers.Fixes #1009, fixes #879 issues w/C and C++ highlighting #1069 Fixes Bug with c++ syntax highlighting on Gitlab #1009, fixes C function name differently highlighted in a declaration and definition #879 issues w/C and C++ highlightingEDIT: Added #1068
EDIT2: Added #1069
The text was updated successfully, but these errors were encountered: