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

Add documentation to hxd.fs package #923

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Yanrishatum
Copy link
Contributor

What the title says. Added documentation to hxd.fs package files.
It'll sit as a draft for a week or so, in case someone would want to fix my grammar or point at other issues.

Extra: Fixed some typos in private function methods of FileConverter

As usual, current up-to-date documentation available here: https://wb.yanrishatum.ru/heaps/api/
Coverage report: https://wb.yanrishatum.ru/heaps/api/coverage.html

public function dispose() {
}

/**
Should be overridden by inheriting class, otherwise throws an exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should document implementation details here, like "Does nothing initially" or "should be overidden".
Basically all these are implementation from FileSystem so we should refer as the interface documentation instead, and introduce only the class in class header.

Note also that BytesFileSystem is not meant to be actually used by user, it's only here as a dummy implementation for hxd.res.Any.fromBytes

-- Same remark for BytesFileEntry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really agree on that. If implementation have specific behaviors or plain up requires user to override the method to utilize the class - it should be noted in the documentation.

I actually always perceived it as a jumpstart to implement your own FS without going too deep into the custom FS specifics, and I do thing it has such an utility, which is portrayed in the docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree in general, but BytesFileSystem is very specific and not really meant to be extended. The good way to implement a FS is to implement the FileSystem interface, that should be the one recommanded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Altered the documentation to explicitly state it's intended for internal use and point at Any.fromBytes, but I've kept the mention of alternative use-case for it.

hxd/fs/Convert.hx Outdated Show resolved Hide resolved
hxd/fs/Convert.hx Outdated Show resolved Hide resolved

Expects `oggenc` to be accessible from command line.

Uses `--resample 44100` and `-Q` flags.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should specify implementation details in source documentation (such as which exe is called and parameters). Maybe we should have instead all these converts refer to online wiki page where we list them and eventually add some details with links to download the software etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think for all default converts I prefer a single class documentation:

/**
   One of the defaults resource converters available, see [link-to-convert-doc] for more details.
**/

@@ -189,6 +202,9 @@ class EmbedFileSystem #if !macro implements FileSystem #end {
this.root = root;
}

/**
Returns the root FileEntry directory of the FileSystem.
**/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, these should only be documented in FileSystem, no duplicate documentation here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I have submit a request to documentation generation tool regarding this
HaxeFoundation/dox#280

hxd/fs/FileEntry.hx Outdated Show resolved Hide resolved
hxd/fs/FileEntry.hx Outdated Show resolved Hide resolved
hxd/fs/LocalFileSystem.hx Outdated Show resolved Hide resolved
hxd/fs/FileSystem.hx Show resolved Hide resolved
hxd/fs/FileEntry.hx Show resolved Hide resolved
@ncannasse
Copy link
Member

Thanks ! I did a full review :)

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