Don't take a HighlightingAssets detour to build assets by Enselic · Pull Request #1802 · sharkdp/bat (original) (raw)

In the current code base, it is necessary to create a temporary HighlightingAssets instance to be able to build assets.

This PR moves the code to build assets to its own place, independent of HighlightingAssets. This allows us to significantly simplify HighlightingAssets since we can make SerializedSyntaxSet mandatory.

This also makes sense on a conceptual level. When building assets, it will never be necessary to also highlight source code. So it makes sense to stop using HighlightingAssets for this.

This means that we no longer allow clients of our API to build assets during startup. But that is very slow, and I can't imagine why anyone would want to do that. And this simplification is worth a lot in terms of flexibility. So IMHO it is worth having to deprecate API.

New public API

I currently put the new API at

• bat::assets::build

to not introduce a new module. Another option I was considering was bat::build_assets. I am open to further ideas.

Deprecated and no longer functional API

• bat::assets::HighlightingAssets::from_files
• bat::assets::HighlightingAssets::save_to_cache

Background information

As I was experimenting with zero-copy deserialization (see #1787) I bumped into the problem of having to use HighlightingAssets to build assets. To support zero-copy deserialization, it will be natural to serialize temporarily owned data. Having to take a HighlightingAssets detour really complicates things.

But IMO this PR has merits of its own, regardless of the work related to #1787.

Status of PR

I'm happy with the code, and consider it ready for CR. But I have not yet done broad verification. I would prefer to get your thoughts on this before I put much time on verification. In case you think this is way too wild :)