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

Code style guidance for new structure #202

Open
hummy123 opened this issue Dec 11, 2022 · 5 comments
Open

Code style guidance for new structure #202

hummy123 opened this issue Dec 11, 2022 · 5 comments

Comments

@hummy123
Copy link

Hi there.

I'm working on a functional AA tree, hoping to add it to this repository once tested, but I ran into a problem related to code style when implementing the IEnumerable interface.

Here is the file: https://github.com/hummy123/Functional-AA-Tree/blob/main/src/AaTree.fs .

The type at the top uses the module's (which comes after the type) toList function, which is enabled by the recursive namespace at the top.

However, this isn't best practice I feel (I like the "onion architecture" style where no piece at the top depends on any code written below), but the other choice I had was to code toList manually without using the fold function defined in the module, which I didn't like either.

I would have liked to add the toList definition and the namespaces at the end of the file where "type AaTree with..." is, but then I get a compile error FS0909 because an interface must be implemented at the start of the type definition. So I'm not sure what to do to contribute in a way that fits this repositories code style (haven't seen "namespace rec" on other files here).

@hummy123
Copy link
Author

hummy123 commented Dec 11, 2022

I've got all the tests passing in my repository (isEmpty, exists, notExists, tryFind, find, toList/Array/Seq, ofList/Array/Seq, fold/foldback, insert, delete) so this is all I'm waiting for before adding the structure here via a pull request.

There is one error using find in the below code:

test "test find" {
            let tree = AaTree.ofList ["hello"; "bye"]
            Expect.equal "hello" <| AaTree.find "hello" tree <| ""
            // Expecto doesn't catch this expected exception for some reason.
            // Expect.throws (fun () -> AaTree.find "goodbye" tree |> ignore) ""
        }

It throws an error as expected, but that error propagates to Expecto's test (Visual Studio breaks the program because of an uncaught error), so the behaviour works but there is one problem with the test code which I am not sure how to handle.

Appreciate your help with the test and/or the code style question.

Link to my repository (not a fork of this) if curious: https://github.com/hummy123/Functional-AA-Tree .

Thanks,
Humza

@gdziadkiewicz
Copy link
Collaborator

Hi Humza, checking.

@hummy123
Copy link
Author

hummy123 commented Dec 11, 2022

Apologies for having put work on your plate over the weekend. 😅 No rush, but I appreciate it whenever you get there.

Edit: I made a new and final commit (everything else is fine from what I see) swapping the names of the fold and foldback functions because I had named them the wrong way around. However, everything is fine now from what I see.

@gdziadkiewicz
Copy link
Collaborator

Hi Humza,
I took a look at your code and it looks promising! Please create a PR for adding AATree to this repo.

After you create it we can work out the details. For starters let's go with the rec namespace. It can always be replaced with something more elegant later. To be transparent - I'm willing to deep check your PR, provide suggestions, work on the kinks and check you implementation with the linked paper, but I can't promise quick response times on my side.

For the unexpected Expecto exception behavior pls check https://github.com/hummy123/Functional-AA-Tree/pull/1 . I suspect that this was something going wrong on your machine as it works as expected in CI and on my machine.

@hummy123
Copy link
Author

Hi Grzegorz.

Thanks for your time and help. Sure, I'll create a PR tomorrow hopefully (almost 10 PM here).

I appreciate your help and don't worry about response times. I think you were right about the Expecto error being a local issue as it isn't happening anymore.

Appreiate your help. Thanks again.

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

No branches or pull requests

2 participants