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

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

Merged
merged 3 commits into from
Nov 18, 2021
Merged

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

merged 3 commits into from
Nov 18, 2021

Conversation

EliteMasterEric
Copy link
Collaborator

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
Owner

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
Collaborator 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
Owner

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
Collaborator 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
Owner

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
Collaborator Author

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

@larsiusprime
Copy link
Owner

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