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

This merges all changes from Increpare up to and including 20-feb-23 #90

Open
wants to merge 141 commits into
base: develop
Choose a base branch
from

Conversation

david-pfx
Copy link

See #23

Ben Small and others added 30 commits January 27, 2022 18:36
Certain loop variables were unscoped, and thereby becoming global; also, a per-level variable (bannedGroup) was not scoped in certain places to the corresponding level object, thus becoming global as well.
(The rules property is an array of arrays, but was getting copied as if it was an array of ordinary data.)
The tokenIndex was being set to 1 for level messages, so the comment handler remained active.  This could cause strange behavior if the message began with a '('.
Fix for increpare#800 (wrong form of URL in standalone homepage link title)

Oh somehow I left out that - must've messed up my commit a while ago.  Good catch. Thanks :)
Using hand-picked/modified things from increpare#798

This change weakens the unit test "EDitor crash with opening parenthesis in level section in middle of line increpare#784" a bit, resulting in a less-precise error message, but I think it still does the trick.
Compiler now uses the parser to parse the legend section rather than having its own very primitive parsing. I hope this doesn't break anything - I should probably throw some more examples with weird unicode at it....

related to increpare#798

[Also includes test-cases, and fixes up some old ones due to me improving error messages, and removed a superfluous/misleading error messages from the lime rick examples]
- Gallery games can be marked as NSFW
- NSFW-rated games don't appear on the puzzlescript.net homepage.
- I've cropped+blurred out the GIFS of any NSFW games and put a visible 'ADULT CONTENT' warning on them.
- Click on the games takes you to an age confirmation page where you have to enter your DOB to see further forwarding links.
the fix to increpare#808 is more about showing a better error in the right place, but I worry about the semantics changing generally here - I should probably make an announcement when I launch it to ask people to let me know if anything's broken! (specifically worried about the bit I commented out in generateExtraMembers )
20 felt better than 10
increpare and others added 27 commits March 14, 2022 12:53
Increpare up to 23/2/22

Conflicts resolved as follows but see parser.js 261-273

#	src/Documentation/css/bootstrap-theme.css
#	src/games_dat.js
#	src/js/addlisteners_editor.js
#	src/js/compiler.js
#	src/js/engine.js
#	src/js/parser.js
#	src/standalone.html
#	src/standalone_inlined.txt
#	src/tests/resources/testdata.js
Use Increpare testing (no merge) but title ...Plus
Use Increpare gallery
Revert/fix some error messages to match tests
Fix old bug when () comment follows message
Run server on changed port, so it can alongside others
Use correct versions of test data files
Passes all tests.
Conflicts fixed:
#	compile.js
#	src/editor.html
#	src/js/gamedat.js
#	src/js/graphics.js
#	src/play.html
#	src/standalone.html
#	src/tests/resources/testdata.js
Conflicts resolved:
#	src/demo/blank.txt
#	src/demo/blockfaker.txt
#	src/demo/byyourside.txt
#	src/demo/constellationz.txt
#	src/demo/kettle.txt
#	src/demo/limerick.txt
#	src/demo/microban.txt
#	src/demo/midas.txt
#	src/demo/naughtysprite.txt
#	src/demo/nekopuzzle.txt
#	src/demo/notsnake.txt
#	src/demo/octat.txt
#	src/demo/randomrobots.txt
#	src/demo/randomspawner.txt
#	src/demo/sokoban_eyeball.txt
#	src/demo/sokoban_horizontal.txt
#	src/demo/sokoban_match3.txt
#	src/demo/sokoban_sticky.txt
#	src/demo/sumo.txt
#	src/demo/whaleworld.txt
#	src/demo/zenpuzzlegarden.txt
#	src/editor.html
#	src/js/parser.js
Fix message parsing to match new PS+
Fix missing PS+ commands
135fb61 7/3/22

Minor changes to pass all tests
Minor changes to tests.js, for test selection

# Conflicts fixed:
#	.build/buildnumber.txt
#	.gitignore
#	README.md
#	src/Documentation/css/bootstrap-theme.css
#	src/Documentation/levels.html
#	src/Documentation/prelude.html
#	src/Documentation/rigidbodies.html
#	src/css/toolbar.css
#	src/demo/constellationz.txt
#	src/demo/kettle.txt
#	src/demo/leftrightnpcs.txt
#	src/demo/midas.txt
#	src/demo/octat.txt
#	src/demo/rigid_scott1.txt
#	src/demo/rigidfail1.txt
#	src/demo/whaleworld.txt
#	src/demo/zenpuzzlegarden.txt
#	src/js/compiler.js
#	src/js/debug.js
#	src/js/debug_off.js
#	src/js/engine.js
#	src/js/inputoutput.js
#	src/js/parser.js
#	src/js/sfxr.js
#	src/js/soundbar.js
#	src/tests/resources/testdata.js
#	src/tests/resources/tests.js
#	src/tests/tests.html
Fix merge bug in handling SECTION levels
Add some debug logging
Merge in plus tests
This merges all changes from Increpare up to and including 20-feb-23

Resolved Conflicts:
#	src/js/compiler.js
#	src/js/parser.js
#	src/tests/resources/errormessage_testdata.js
#	src/tests/resources/plus_errormessage_testdata.js
#	src/tests/resources/plus_testdata.js
#	src/tests/resources/tests.js
@Auroriax
Copy link
Owner

  • Please elaborate a bit more on decisions you made during merging and things you tested, right now I see a lot of commit names like "Merge commit 'HASH' into tomerge" that aren't clear for me.
  • These are too many commits to review for a single PR. Please submit the PS release merges one-by-one as PRs, as originally discussed in Get latest changes from Increpare #23 (comment)
  • You also seem to be merging in a lot of fixes unrelated to the merge that I haven't seen reported as issues. PRs are usually feature branches, if you want to make other changes then please submit these as separate issues and PRs. Unless required, in which case please elaborate.
  • Right now your "merge" commit seems to merge a lot of changes at once. I'd expect to have a merge for every 5 to 10 commits as originally discussed in Get latest changes from Increpare #23 (comment), but there seem to be less frequent. Did you already squash these, perhaps?

@david-pfx
Copy link
Author

I don't think I can help there. For most of these changes I have no idea what the original issue was -- you'll have to go look at the PS issue numbers if you care. This is what you get when you want to track the parent of your fork, and you leave it such a long time between updates. This is not like a typical PR, and I don't think you can treat it as one.

I followed your suggestion and used an imerge-like process to find and resolve the merge choke points, but the merges are all mechanical, not based on any consideration of what they do. I'm not interested in why Stephen did what he did, I just want a clean final result and that's what this is. Remember: this code is his, not mine.

As you know, you were unable to help me with a mechanism for submitting PRs, given that I already had a PS fork, I came to the conclusion that leaving it in pieces and submitting them separately would have resulted in a final review load much the same, and breaking it into pieces now would add work for me and not save work for anyone.

All I can suggest is that you review the critical files (parser, compiler, etc) looking for blunders. If you find anything you don't like or understand ask me or fix it yourself. Then hopefully we reach a position where you're confident enough to merge it onto your develop branch, and in due course onto master. Your call. I'm just here to help.

@david-pfx
Copy link
Author

This PR has been stuck here for a while. What do we have to do to get it moving?

Please accept that this is not a normal PR: it's a catch up of changes made by increpare. I am not responsible for the issues/features/bugs or whatever they address, and I am not able to debate them. If you wanted to curate individual changes you should have done that before I started. My remit was precisely as per the issue title: 'Get latest changes from increpare', and that I have done. The code works, it passes all the tests, it doesn't break anything, it does what it says on the tin.

I'm happy to put in a bit more work if it will help, but right now I just don't know what you want. It seems you've always done this yourself in the past, and it's not obvious how to deal with a big swatch of changes like this. Your call.

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.

5 participants