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

LS-proper-proper-fix-I-swear #164

Merged
merged 3 commits into from Oct 28, 2021

Conversation

fredg1
Copy link
Contributor

@fredg1 fredg1 commented Oct 28, 2021

:,(

@fredg1 fredg1 requested a review from a team as a code owner October 28, 2021 00:36
Copy link
Contributor

@heeheehee-kolmafia heeheehee-kolmafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also generate a debug log with the exciting script

script;

I suggest being more methodical than "hey look script validation no longer fails on this specific case".

@fredg1
Copy link
Contributor Author

fredg1 commented Oct 28, 2021

I can also generate a debug log with the exciting script

script;

I suggest being more methodical than "hey look script validation no longer fails on this specific case".

I tried to be thorough; I looked at everything calling makeComment and makeToken, and saw that specific case.
It's just that I focused on the endIndex = line.length();, and didn't think of also looking into the endIndex = line.indexOf(";"); when I ruled out that the former was safe ;-;

@fredg1
Copy link
Contributor Author

fredg1 commented Oct 28, 2021

Please don't think I'm botching this ;A;

Empty directives should probably be an error...
Copy link
Contributor

@heeheehee-kolmafia heeheehee-kolmafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are all the changes we need. Might be nice to have a second opinion, though.

@BadHorseMonkey?

@BadHorseMonkey
Copy link
Contributor

I think these are all the changes we need. Might be nice to have a second opinion, though.

@BadHorseMonkey?

Let me fire up my Fredg directory and check out the branch...

@BadHorseMonkey
Copy link
Contributor

BadHorseMonkey commented Oct 28, 2021

I think these are all the changes we need. Might be nice to have a second opinion, though.
@BadHorseMonkey?

Let me fire up my Fredg directory and check out the branch...

builds for me. I ran

cli_execute( "validate VeracitySpacegate.ash" );
print( "veracity0-spacegate" + ": " + svn_exists( "veracity0-spacegate" ));

on 28 scripts in my scripts directory. I'm currently running my Autoscend evening turns in it. Nothing I know of is broken.

It occurs to me that if we're revamping the parser, one thing we'll want is a library of good and bad files to test with, which can prove that our parser is acting correctly end-to-end. It wouldn't be unit tests, but complementary to them. Functional tests are good at finding things like this.

@heeheehee-kolmafia
Copy link
Contributor

Let me fire up my Fredg directory and check out the branch...

fyi you can fetch PR branches via git:

git fetch origin pull/164/head:new-branch-for-the-pr
git switch new-branch-for-the-pr

Anyways. I'll merge this fix, then. I couldn't find anything else when staring at the code.

@heeheehee-kolmafia heeheehee-kolmafia merged commit e4a5ee4 into kolmafia:main Oct 28, 2021
@fredg1 fredg1 deleted the LS-proper-proper-fix-I-swear branch October 28, 2021 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants