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

fixed parsing of scenario matches #101

Merged

Conversation

Skazu
Copy link
Contributor

@Skazu Skazu commented Jul 17, 2023

Created dedicated source branch for the issue mentioned here:
#100
because i have additional fixes for another problem.

I'm closing the old PR.

Open in this PR:
Fixes for the Full-Body-Parser.

@Skazu
Copy link
Contributor Author

Skazu commented Jul 20, 2023

FYI: I've added code to parse triggers/conditions for scenario records but for DE only, it doesn't break the old behaviour for aoc and others, and of course it's still in the fast header parser only.
There's also an example record commited.

I don't currently know if I will port the code into the full header parser, it would cost me quite a lot of time, the declarative approach and the long test duration makes it very difficult and unpleasant to debug there.
But I would still like to let you profit from our adaptations upstream. Maybe someone else who uses the full parser has the time to port the code?

Let me know if you have any suggestens for the code.

@happyleavesaoc
Copy link
Owner

It's fine to support only the fast header. As you say, anyone who wants can port it back given this example.

But, please make the tests pass (I guess by excluding the scenario file from the full tests).

@Skazu Skazu force-pushed the fix/scenario-parsing branch 2 times, most recently from b54a45b to 00ade98 Compare July 25, 2023 08:11
@Skazu
Copy link
Contributor Author

Skazu commented Jul 25, 2023

I have made a few code corrections and fixed the tests. From my point of view, the code is now ready-to-merge.

@@ -32,5 +31,50 @@ def test_files_full(self):
parse_file_full('tests/recs/de-13.07.aoe2record')

def test_files_fast(self):
for path in glob.glob('tests/recs/*'):
Copy link
Owner

Choose a reason for hiding this comment

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

Let's do this:

skip = {"tests/recs/de-50.6-scenario.aoe2record", ...}
for path in glob.glob('tests/recs/*'):
    if path in skip:
        continue
    parse_file_fast(path)

That way we don't need to update this test every time we add another test file.

@Skazu
Copy link
Contributor Author

Skazu commented Jul 27, 2023

I've changed it.

@happyleavesaoc happyleavesaoc merged commit c479f89 into happyleavesaoc:master Jul 27, 2023
1 check passed
@happyleavesaoc
Copy link
Owner

Thanks!

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.

2 participants