1703: Introducing MapExtensionToUnknown MappingTarget

[cbolgiano]

Resolves #1703

[cbolgiano]

Please I need guidance on what to do with the failing regression test for resolv.conf. Is the test valid anymore without the explicit mapping, can it be remove, or what can we do to fix the failing test?

[Enselic]

The failing test is a real failure. You can reproduce it locally like this:

Step-by-step:
cargo run -- tests/syntax-tests/source/resolv.conf/resolv.conf

Expected:
Colors

Actual:
No colors

Not sure why, you'll have to debug it :)

But, I would expect us to have to split up get_extension_syntax() into two different helper functions, that each call try_with_stripped_suffix, so that we would have the following order when finding a syntax:

1. MapToUnknown (can still be first to minimize risk of regressions)
2. MapToSyntax (can still be second to minimize risk of regressions)
3. Call self.find_syntax_by_extension with the file name
4. MapToUnknownUnlessExactFileNameMatch
5. Call self.find_syntax_by_extension with the file extension
6. First line match

Something like that. Thanks again for taking a shot at solving this in a generic way!

[Enselic]

After thinking about it some more, MapExtensionToUnknown is probably a better name

[cbolgiano]

Thank you for the guidance and patience. I look forward to giving this another shot.

[cbolgiano]

I found out where the issue was in the regression tests. Pushing up a fix now.

[cbolgiano]

Not quite there yet. Will continue to investigate.

[Enselic]

In case it helps, here are the commands I personally use to simulate our CI pipeline. Regular tests:

% echo bat ci && cargo fmt -- --check && cargo doc --locked --no-deps --document-private-items --all-features && cargo clippy --all-targets --all-features && cargo test --doc && cargo test && PATH="./target/debug:$PATH" tests/syntax-tests/test_custom_assets.sh && echo ci pass

Syntax regression tests:

cargo build --no-default-features --features minimal-application,build-assets && PATH="./target/debug:$PATH" assets/create.sh && cargo build --release --no-default-features --features minimal-application,build-assets && PATH="./target/release:$PATH" tests/syntax-tests/regression_test.sh

If the above commands pass locally, you can be pretty sure that our CI pipeline in GitHub Actions will also pass.

[cbolgiano]

That helps greatly. Thank you.

Going to take some time to better understand the code and attempt again this weekend.

[cbolgiano]

The comments for the get_syntax_for_path, the guidance on the desired order, and the knowledge on how to run the regression tests like the CI locally really helped. Thank you.

[Enselic]

Thanks for your continued work on this! Getting better and better each time.

[cbolgiano]

Pushed.

[cbolgiano]

I ran regression tests but not integration tests locally this pass. Need to resolve the single failing integration test.

[cbolgiano]

I have the integration tests passing locally but now a different set of regression tests are failing. The Ignored suffixes regression tests are failing. I am going to push what I have so far. To get more eyes on where I am at.

[cbolgiano]

I have good news. I was able to get the regression and integration tests to pass locally. Now I will be pushing up the latest changes.

[Enselic]

Thanks for the updates. Just wanted to let you know that I have noticed your review request. Might take a while before I find the time to do a detailed review, however.

I glanced it over, and on a high level it looks good. But I think we can polish the new if-case code some more. While waiting for review, feel free to fix the lint warnings that clippy (the Rust linter) finds. You can see the lints in the "Files changed" tab in the PR.

[cbolgiano]

Thanks for the updates. Just wanted to let you know that I have noticed your review request. Might take a while before I find the time to do a detailed review, however.

I glanced it over, and on a high level it looks good. But I think we can polish the new if-case code some more. While waiting for review, feel free to fix the lint warnings that clippy (the Rust linter) finds. You can see the lints in the "Files changed" tab in the PR.

Pushed changes to address issues found by linter.

[cbolgiano]

Regression and integration tests pass locally. Pushing up now.

[cbolgiano]

Pushed latest changes. Please feel free to update documentation, test, or changelog as needed. This was a fun problem to work on. Thank you for all the time! Please do not forget PR #1882 I'd be happy to make any changes needed there after this PR has been merged.

[Enselic]

I made some final adjustments in a commit of my own since I figured that would be easier than commenting. Do you think the changes look ok?

In any case, I consider this code Approved now! 🎉 Thanks a lot for all the help!

I will let this PR remain open for some time to give other maintainers a chance to look at it before it is merged.

[Enselic]

@sharkdp @keith-hall I plan to (squash and) merge this early next week. Let me know if you would like more time to look at it.

[sharkdp]

Excellent work - thank you very much to everyone involved.