Skip to content

Adding Classes to scarpet. #306

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

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Adding Classes to scarpet. #306

wants to merge 26 commits into from

Conversation

Ghoulboy78
Copy link
Collaborator

@Ghoulboy78 Ghoulboy78 commented Apr 3, 2022

This is a continuation from this discord conversation, and the code I wrote there.

In this PR, I also rename heap.sc to min_heap.sc, and add max_heap.scl as a showcase of what could be done with classes.

NB: This PR should be Squashed and Merged, to avoid adding a bunch of useless commits to the history.

Todo:

  • Examples, basically, in Readme.md

@Ghoulboy78 Ghoulboy78 added enhancement New feature or request new-app About adding a new app labels Apr 3, 2022
@Ghoulboy78 Ghoulboy78 requested a review from gnembon April 3, 2022 11:16
@Ghoulboy78 Ghoulboy78 mentioned this pull request Apr 3, 2022
5 tasks
@Ghoulboy78 Ghoulboy78 requested review from altrisi and BisUmTo April 5, 2022 06:52
BisUmTo
BisUmTo previously approved these changes Apr 5, 2022
Copy link
Collaborator

@BisUmTo BisUmTo left a comment

Choose a reason for hiding this comment

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

I spent 1 hour learning how should max_heap actually work 🤣

The classes.scl is really great, I'll surely use it for my projects

So we cane actually make multiple different instances
@BisUmTo
Copy link
Collaborator

BisUmTo commented Apr 5, 2022

Why do you need to copy the class? 👀

@Ghoulboy78
Copy link
Collaborator Author

Because otherwise you end up just having 1 map and so one object, instead of multiple instances of the same class of object. Taking a copy means that the original is unchanged and the new_object method returns a separate map to the one we input.
I came across this problem when rewriting matrix as a class to make it easier, and then realised that I was having this issue.

@Ghoulboy78 Ghoulboy78 requested a review from replaceitem April 7, 2022 06:06
@Ghoulboy78
Copy link
Collaborator Author

Ghoulboy78 commented Apr 7, 2022

BTW Im working on inheritance rn. It's not too hard and I've got it mostly working already, just need to figure out whether inheriting a class which already inherits from another class works or not.
Definitely parametric classes won't work (At least I think that's what they're called? I'm talking about things like class MyClass<A> or something like that)

@Ghoulboy78
Copy link
Collaborator Author

Ok so I've finished inheritance. I will be adding extensive documentation to Readme.md cos currently it's got like 1 line tbh.

@Ghoulboy78
Copy link
Collaborator Author

I'm going to make all classes have a couple fundamental methods, which I will put into an 'object' class

@BisUmTo BisUmTo dismissed their stale review April 10, 2022 17:05

Too many changes, I didn't test last release

@Ghoulboy78 Ghoulboy78 marked this pull request as draft April 10, 2022 19:55
@Ghoulboy78 Ghoulboy78 marked this pull request as ready for review May 18, 2022 12:50
Copy link
Contributor

@opsaaaaa opsaaaaa left a comment

Choose a reason for hiding this comment

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

Looking at the code. I think this is good for what it is!
It looks thought out. I'm not seeing any obvious better approaches in the few minutes i've thought about it. (native built in scarpet classes might be nice, but also i wouldn't want to write that...)

using call() for everything feels clunky, but thats how you have to do it.

I think large scripts are the once's that would benefit from this.
I remember bemoaning not having classes while writing the spellbook script.
Ill try this out more thoroughly if/when I rewrite it. (or any other larger scripts)

@Ghoulboy78 Ghoulboy78 requested a review from BisUmTo July 17, 2022 06:22
@Ghoulboy78 Ghoulboy78 marked this pull request as draft August 9, 2022 02:52
@Ghoulboy78
Copy link
Collaborator Author

Still gotta figure out /script download stuff

@Ghoulboy78
Copy link
Collaborator Author

Yeah, I don't think the /script download stuff is so important, I'm just gonna put this up for review as it is, cos other stuff depends on it.

@Ghoulboy78 Ghoulboy78 marked this pull request as ready for review January 23, 2023 14:15
@Ghoulboy78 Ghoulboy78 requested a review from opsaaaaa February 2, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new-app About adding a new app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants