Skip to content

Fully abstracted file system, plus code quality improvements.#34

Merged
EliteMasterEric merged 3 commits intoFunkinCrew:masterfrom
EliteMasterEric:feature/various-fixes
Nov 18, 2021
Merged

Fully abstracted file system, plus code quality improvements.#34
EliteMasterEric merged 3 commits intoFunkinCrew:masterfrom
EliteMasterEric:feature/various-fixes

Conversation

@EliteMasterEric
Copy link
Member

This PR performs the following changes:

  • Replaced static access to PolymodFileSystem with access to an instance of IFileSystem. This defaults to SysFileSystem if available.
  • Added a parameter to allow developers to pass a custom IFileSystem, similar to IBackend.
    • Developers should now be able to implement Arbitrary virtual filesystems #12 on their own, although built-in implementations of stuff like zip modloading would be cool.
  • Added a VSCode config to tell users to install the Haxe extensions for code formatting and syntax highlighting.
  • Added a HaxeFormatter config and applied it to all files.

@larsiusprime
Copy link
Collaborator

I really appreciate this commit! Looks like a lot of hard work went into it.

There's two things that keep me from merging it right away:

  1. The reformatting is nice and probably something I should adopt, but it makes it hard to evaluate the diffs. Is it possible to revert those for now and save formatting changes for a future PR? That way I can more easily tell what files and functions have actually changed.

  2. I'm in the middle of accepting another PR that adds Electron support and I once I make sure that can be merged without breaking stuff, then I'll turn my attention here. Hopefully it won't add too much complication to what you're doing.

@EliteMasterEric
Copy link
Member Author

EliteMasterEric commented Sep 16, 2021

Hey Lars,

I wanted to say thank you for all the work you've put into this project (especially recently, I have a basic implementation of the latest changes working in Friday Night Funkin' already). It's very powerful and flexible as is, and every bit of effort you put into making it better is much appreciated.

In terms of the code formatting changes, I put them in their own commit, but I realize now that the "Files changed" view doesn't let you filter by that. I'll move them to their own PR. (EDIT: That's apparently easier said than done, Git HATES working with two branches with different formatting.)

As for the Electron PR, I can test merging the branches together and see how that plays out.

@larsiusprime
Copy link
Collaborator

Thanks for working with me here :) I also just realized that some of my samples are out of date, such as the Heaps demo. Lot of maintenance work to catch up on, and I don't even have any unit tests, I just rely on the samples telling me if something seems broken or not.

@EliteMasterEric
Copy link
Member Author

Hey, I've force pushed a new commit which removes the formatting changes.

As for the Electron support, it appears that the two changes are absolutely not compatible due to major merge conflicts. @TamarCurry adds a new NodeFileSystem but doesn't implement the interface I created here.

For this to work, we need to have one developer's changes be merged and have the other rework their PR to be compatible.

@larsiusprime
Copy link
Collaborator

Yeah, I figured as much. So I promised Tamar that I would look into his first as he's been waiting patiently for over a week now. I may need to get directly involved to deal with the cleanup work to reconcile his with yours, if you're okay with that.

@EliteMasterEric
Copy link
Member Author

That's fine with me, as long as it doesn't put too much work on your plate. 😄

@larsiusprime
Copy link
Collaborator

Eh, all in the day of a life of an open source maintainer. If you'll be patient with me we'll get this sorted eventually.

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