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

Some first maintenance work #78

Closed
wants to merge 54 commits into from

Conversation

splitbrain
Copy link
Contributor

This is the first work on what is outlined in #77

This takes care of nearly all open pull requests (I left comments on those that I think shouldn't or can't be merged), fixes all the problems detected by langcheck (actual users of those languages are encouraged to check and submit update PRs) and also removes some PHP4 cruft.

okkero and others added 30 commits January 28, 2017 16:47
new builtins
Uses posted list of Mathematica 11 functions; changed to one-per-line
(to make diff-ing easier)
Changed to Mathematica 11 list; updated to one-per-line
Usefull for links to wiki pages
Added tcl-commands dedicated to eggdrops and binds.
Links to wiki.eggdrop.fr, because eggwiki.org is not enough filled yet.
this makes merging changes easier later
The comment styling was weird. This attempts to fix the actual problems
but might not be the styling that was wanted. This should be checked by
@jonathanbennett73 or other autoit users
This removes the GESHI_PHP_PRE_433 constant and the related workarounds.
This adds the missing variable names to the doc blocks, fixing a lot of
problems found in PHPStorm
I'm using protected instead of private to let programmers inherit from
GeSHi and overwrite internals as needed.
These should be accessed through the proper methods only.
I forgot to change the checks to check against the new variable
this allows IDEs to pick up a very basic coding standard
more checks from contrib langcheck need to be converted
This avoids loading the language file multiple times and it is easier
frustrating to fix many errors at once.

This copies over a lot of functionality from the langcheck.php script in
contrib. Ideally that script should make use of the LangCheck class
instead.
There actually seems to be a problem where the highlighter stops at a
regexp in the middle of the source! I commented out that assertion for
now.
This allows to run tests against a single datafile as described in
https://phpunit.de/manual/5.7/en/textui.html

Eg to run the test case against the java language file you can invoke
phpunit like this:

./vendor/bin/phpunit --filter 'test_langfile with data set "java"'
now has much less duplicated code
…rnize

* 'master' of https://github.com/GeSHi/geshi-1.0:
  Make langcheck set an exit code on error and warning
@splitbrain
Copy link
Contributor Author

Hmm, dependencies can't be satisfied because phpunit 5.7 does not support PHP < 5.6. Do we really need to test on those ancient versions?

@cweiske
Copy link
Member

cweiske commented Mar 14, 2017

No, we don't.

@cweiske
Copy link
Member

cweiske commented Mar 14, 2017

The new langcheck does not catch all errors as the old one did. See my branch "newtest" which tries to get the same results on the current master files as the old one did. I'll continue to work on it.

@splitbrain
Copy link
Contributor Author

splitbrain commented Mar 14, 2017

Hmm the checks are more or less copied over from the old langcheck.php, but I will have a look later today as well.

Also the old langcheck.php did return some wrong errors. Eg. it reported that there is no author and copyright set for some files, even though there clearly was.

@cweiske
Copy link
Member

cweiske commented Mar 14, 2017

Ok, I've fixed the issues in LangCheck and pushed them to master. travis + langcheck fix picking will follow later today.

@splitbrain
Copy link
Contributor Author

@cweiske Okay I need a bit of guidance here (hopefully in a more than a one sentence answer). You seem to be reluctant to ever merge this pull request completely and instead pick single commits. Which is fine I guess if I'd know that's what is going on.

My goal was to get to a point where the backlog of open pull requests can be closed and where the project can get a new stable tag for composer. So people have an official source to use in their projects.

Ideally I would want to see this project revived to a point where at least trivial additions (like new languages) can be added quickly again (as it used to be a few years ago). Is that a goal you would agree with?

Is this pull request useful to you? Does it even make sense for me to merge master back into it again, just to keep it mergable? Is my previous and future work as outlined in #77 even wanted?

I kept the autoloading bootstrap mechanism in phpunit because IMHO it's
the right way(tm)

* upstream/master:
  langcheck fixes for standardml
  langcheck fixes for gml
  langcheck fixes for css
  css: out each keyword on it's own line
  langcheck fixes for ceylon
  langcheck fixes for asymptote
  Add configuration for travis-ci.org
  Introduce langcheck testing via phpunit.
@cweiske
Copy link
Member

cweiske commented Mar 14, 2017

My goal is to support you get the big backlog of patches into master, and a new release out. I'm partially self-motivated because my local last-public-release-installation of geshi throws warnings on php7.

As a maintainer, I look at each single commit. I will not merge a huge pack of commits and hope for the best.
I also try to merge patches strategically: Basework like making automated tests work is first, so that all following merges will automatically get checked by them. This is the reason that the langcheck patches were the first that got merged, and the travis config second. Then the patches necessary to make the tests green were merged.

Now that the foundations are partially there, the boring changes were merged: docblocks, php4 removal, property/method visibility.
After that is done, the new languages and language updates will get merged.

But I already told you that I squash multiple commits into single ones, e.g. "new language (phix)", "phix updates" and "langcheck fixes for phix" will become one. That way history will not be cluttered with useless commits.

Is this pull request useful to you?

Yes, I use it as todo list of which commits need to be merged into master.

Does it even make sense for me to merge master back into it again, just to keep it mergable?

I actually have a local branch based on yours that I constantly rebase whenever master changes. So you don't have to keep it mergable since I do it already.

Is my previous and future work as outlined in #77 even wanted?

I appreciate your work, as said above it helps me tremendously.

I'm not sure if I want everything you wrote down there, especially moving docs out of the repository. API docs can be dropped (because they can be autogenerated), but usage and so should stay in the repository itself. geshi has seen CVS, SVN and now git on github, and I guess that github will not be the last place it lives. Having the docs outside the repository makes moving to the next system harder.

Also, I'll have to check back with @BenBE if sourceforge releases should stay or not. The PEAR channel is still there, and I would like to keep it. Removing the old build scripts would not be good for that.

But I also want new release at the end.

@splitbrain
Copy link
Contributor Author

Okay I can live with this.

I don't necessarily agree on the docs though. The current document is a hard to maintain HTML file that seems not to have been touched significantly in the last 10 years. Moving it to the github wiki makes it easier to edit (because markdown) and opens it for easy additions by users. Wiki contents can easily be checked out via git (https://github.com/GeSHi/geshi-1.0.wiki.git) so you're not risking that documentation is siloed into github should you wish to migrate to a different service later on.

If you insist on keeping it in the source I would at least recommend splitting it into multiple files and maintain it in markdown format. HTML could then produced alongside the API doc generation.

@cweiske
Copy link
Member

cweiske commented Mar 15, 2017

All the commits listed here have been merged into master now. Thanks.

@cweiske cweiske closed this Mar 15, 2017
@splitbrain splitbrain deleted the modernize branch October 17, 2019 19:07
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.