-
Notifications
You must be signed in to change notification settings - Fork 78
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
Comments
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:
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, |
Hi Humza, checking. |
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. |
Hi Humza, 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. |
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. |
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).
The text was updated successfully, but these errors were encountered: