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

Initial apt parsing #12

Merged
merged 9 commits into from Dec 24, 2017

Conversation

Projects
None yet
2 participants
@feliwir
Collaborator

feliwir commented Dec 23, 2017

Added most of the basic character parsing and added two tests, to verify that apt loading is working as expected

MovieName = name;
}
public void Parse(BinaryReader reader)

This comment has been minimized.

@tgjones

tgjones Dec 23, 2017

Collaborator

Could this be a private method? I don't think it's called from anywhere other than AptFile.FromFileSystemEntry.

internal string MovieName;
internal FileSystem FileSystem;
public AptFile(ConstantData constants, FileSystem filesystem, string name)

This comment has been minimized.

@tgjones

tgjones Dec 23, 2017

Collaborator

Can this be a private constructor?

else
{
var importEntry = FileSystem.GetFile(Path.ChangeExtension(import.Movie, ".apt"));
importApt = AptFile.FromFileSystemEntry(importEntry);

This comment has been minimized.

@tgjones

tgjones Dec 23, 2017

Collaborator

Presumably this could result in parsing an .apt file multiple times, if it gets imported into multiple other files? That's why I was thinking this resolving of imports / exports would happen outside AptFile, in a separate layer that can make sure .apt files only get parsed once. What do you think?

This comment has been minimized.

@feliwir

feliwir Dec 23, 2017

Collaborator

In theory that can happen, but parsing doesn't really require a lot of time. My idea was to have FromFileSystemEntry load an entire finished .apt, so you can't possibly end up with a crippled AptFile

This comment has been minimized.

@tgjones

tgjones Dec 24, 2017

Collaborator

I like the goal of not being able to have an incomplete AptFile. Let's leave it as it is, but perhaps in the future we can pass importDict into AptFile.FromFileSystemEntry, so they can be cached globally.

public static Button Parse(BinaryReader reader)
{
var b = new Button();

This comment has been minimized.

@tgjones

tgjones Dec 23, 2017

Collaborator

I'd prefer if you used full words for local variable names, like button here. I know there are several different schools of thought on this, but that's what I've done in the rest of the code.

ch = Movie.Parse(reader, container);
}
else
return null;

This comment has been minimized.

@tgjones

tgjones Dec 23, 2017

Collaborator

A tiny thing, but please could add braces around this else branch too?

@@ -15,4 +16,12 @@ static BinaryUtility()
/// </summary>
public static Encoding AnsiEncoding { get; }
}
internal abstract class BinaryObject

This comment has been minimized.

@tgjones

tgjones Dec 23, 2017

Collaborator

Is this used anywhere?

@feliwir

This comment has been minimized.

Collaborator

feliwir commented Dec 23, 2017

Did fix the mentioned issues

@tgjones tgjones merged commit 80c2758 into OpenSAGE:master Dec 24, 2017

@tgjones

This comment has been minimized.

Collaborator

tgjones commented Dec 24, 2017

Thank you! I did a "squash" merge, to combine the commits into a single commit, so you'll want to get your branch in sync with upstream before carrying on :)

@tgjones tgjones added this to the v0.1.0 milestone Apr 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment