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

Implemented composite pattern for filesystem #256

Merged
merged 35 commits into from
Apr 20, 2023

Conversation

schlingling
Copy link
Contributor

@schlingling schlingling commented Apr 10, 2023

closes #131

extends #123 and #219

Introduces a clean composite pattern for all filesystem related artefacts like it is done in ADAP.

image

The naming convention of the .ts-files yields the class-hierachy, so it is easier to understand.

image

@schlingling schlingling self-assigned this Apr 10, 2023
@schlingling schlingling marked this pull request as ready for review April 12, 2023 17:44
@schlingling
Copy link
Contributor Author

@rhazn FYI, since you are on leave, i requested Felix for an Review :)

@rhazn
Copy link
Contributor

rhazn commented Apr 12, 2023

Thank you, if this is still open next week ping me and I can take a look then as well. If Felix is fine with it that's enough as well of course.

Copy link
Contributor

@felix-oq felix-oq left a comment

Choose a reason for hiding this comment

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

To avoid the instanceof checks, consider introducing new methods in FileSystemNode that perform the recursive descents. Those can then be called in the FileSystem class.

Also I think that paths should generally be treated case-sensitive.

libs/extensions/std/exec/src/file-picker-executor.ts Outdated Show resolved Hide resolved
libs/execution/src/lib/types/io-types/filesystem-node.ts Outdated Show resolved Hide resolved
libs/execution/src/lib/types/io-types/filesystem.ts Outdated Show resolved Hide resolved
libs/execution/src/lib/types/io-types/filesystem.ts Outdated Show resolved Hide resolved
@schlingling

This comment was marked as outdated.

schlingling and others added 9 commits April 17, 2023 19:56
Co-authored-by: Felix Quast <51856713+felix-oq@users.noreply.github.com>
Co-authored-by: Felix Quast <51856713+felix-oq@users.noreply.github.com>
Co-authored-by: Felix Quast <51856713+felix-oq@users.noreply.github.com>
Co-authored-by: Felix Quast <51856713+felix-oq@users.noreply.github.com>
@schlingling
Copy link
Contributor Author

schlingling commented Apr 18, 2023

To avoid the instanceof checks, consider introducing new methods in FileSystemNode that perform the recursive descents. Those can then be called in the FileSystem class.

Also I think that paths should generally be treated case-sensitive.

all implemented with new commits including Design for uniformity.

Copy link
Contributor

@felix-oq felix-oq left a comment

Choose a reason for hiding this comment

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

A few more hints for improving the implementation, but otherwise this is good to go.

@schlingling schlingling merged commit 5803cb6 into main Apr 20, 2023
@schlingling schlingling deleted the feat-filesystem-as-compositum branch April 20, 2023 07:28
@schlingling schlingling restored the feat-filesystem-as-compositum branch April 20, 2023 07:28
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2023
@schlingling schlingling deleted the feat-filesystem-as-compositum branch April 20, 2023 07:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor FileSystem to Compositum
4 participants