Comments (23)
I feel the same about the lack of struct
Im not sure about irust/mod.rs but maybe if we create more independent structs the number of impl blocks would decrease, and would make this change more desirable
just a small example diff would be great so I can see it better
Also, I think I’ll tackle points 2 and 3 if that’s alright with you
Thanks a lot! Ill probably start with 1
Btw GNvim is awesome!
from irust.
. . . just a small example diff would be great so I can see it better
@sigmaSd here you are (refactoring cursor.rs and the Cursor
struct, and in a gist since it's a bit big):
https://gist.github.com/smolck/1baaeb11cf42f7f9e64baa84a730900b
The point I'm trying to make is that each struct should handle it's own things: example being Cursor
handling itself (and the TerminalCursor
too since IMHO that makes sense, as shown in the diff). To me it feels better and more ergonomic to have IRust
call functions on its members directly via self.foo_member.foo()
(as opposed to self.foo_for_foo_member()
). And if we really need to access the IRust
instance, we can pass the function in question a reference to IRust
. If any of that (or the following) was unclear, please let me know.
What this means is that in each file (e.g. printer.rs and others), we remove the impl IRust
and replace it with the corresponding struct (e.g. impl Printer
and others), or implement the corresponding function(s) directly in IRust
if it (or they) belong there.
I'll work on making the above diff function correctly with the rest of the codebase (it requires some changes to the IRust
struct, like removing the internal_cursor
field, in addition to changing some function calls), assuming you like it.
from irust.
Love the design!! I really like the idea of taking irust by ref, now we can actually have real independent structs, also integrating crossterm cursor in cursor makes so much sense!
Thanks a lot for doing this!
from irust.
@smolck would you push your changes to Cursor? I have a big rework coming up https://github.com/sigmaSd/IRust/tree/wip_rework_printing_input and I want to use your changes on the Cursor
What this refactor does:
1- addresses issue "1-" (printing the whole buffer instead of the inputted character)
2- add highlighting input
3- Remove a lot of the complexity of Cursor: In particular it removes all the bounds logic
from irust.
I think if we manage to merge writer and printer successfully, making the new struct own the cursor is a nice idea!
However would you hold on working on it for a while I'm working on https://github.com/sigmaSd/IRust/tree/wip_rework_printing_input it will change a lot of the code base, hopefully I'll be finished soon, so when I'm done you won't need to rebase
from irust.
However would you hold on working on it for a while I'm working on https://github.com/sigmaSd/IRust/tree/wip_rework_printing_input it will change a lot of the code base, hopefully I'll be finished soon, so when I'm done you won't need to rebase
Sure! Thanks for the heads up! Just @ me when you’re ready for me to start working on it.
from irust.
Ok thanks as well!
from irust.
@smolck I published the 0.7.0 version, it still have some issues but highlighting input is so cool I really wanted to ship this
And let me be honest here I probably made the code-base even messier xD
But from now on at least there isn't a major feature I really want right now, so I can focus on refactoring, docs and tests to make this project more contributor friendly
Hopefully it works out!
from irust.
@sigmaSd Hello again! Sorry about not contributing lately, been working on other stuff. Hopefully I can get a PR to merge the Printer and Writer structs soon.
One question: could you add another bullet point to the top comment for tests? Those will be important for future development, so I figure they deserve a spot up there.
EDIT: #45 does exist, so if you do the above maybe link to it. Also, would you mind listing the PR numbers next to each bullet point that fix (or are attempting to fix) each corresponding issue? Helps us know what PR fixed each issue and/or what is being worked on.
from irust.
Hi again @smolck ! you're welcome at anytime!!
I added your suggestions, just so you know I attempted to merge printer and writer but I couldn't figure the right abstraction to encapsulate them, I hope you have a better luck xD
from irust.
@sigmaSd What do you think of separating much of the logic into separate structs? For example, writer.rs
contains an impl block for IRust
, when to me it feels like it would have a Writer
struct inside. I think the IRust
struct may have too many jobs that it should delegate to other structs instead. Even though the file may be long, a single irust.rs
/mod.rs
file for IRust
’s impl block would probably make the codebase more readable, and remove the impl blocks spread out everywhere. An example of what I mean is the GNvim project’s file structure, (I’m a bit biased being a contributor there, but still; it’s a frontend GUI for Neovim BTW). If you want an example diff for how this would look in IRust I’d be glad to come up with one.
Also, I think I’ll tackle points 2 and 3 if that’s alright with you.
from irust.
Thanks a lot for doing this!
No problem! Thank you for welcoming the change(s)!
As far as how I’ll go about the refactor, I think doing it in one branch with multiple commits is probably the most reasonable (i.e. one commit for refactoring Cursor
, one for Printer
, etc.); what do you think?
from irust.
I think that would be the most reasonable way too
Btw the problem with taking irust by ref is we will face borrow rules, self refs is a no go in rust, do you have a solution for this?
from irust.
. . . self refs is a no go in rust
Not sure I understand this. You can pass a reference to self
around in Rust (is that what you meant?). You can paste code like this into a *.rs file and it'll work:
struct FooStruct {
some_foo_data: Vec<i32>,
}
struct OtherStruct {
pub more_data: Vec<i32>,
}
impl OtherStruct {
pub fn calls_foo_struct_func(&self) -> i32 {
// Passes self as param, valid Rust code.
FooStruct::takes_struct_by_ref(self)
}
}
impl FooStruct {
// Associated func.
pub fn takes_struct_by_ref(instance_of_other: &OtherStruct) -> i32 {
// Use data from `instance_of_other`
instance_of_other.more_data.iter().fold(0, |acc, x| acc + x)
}
}
fn main() {
let other_foo = OtherStruct { more_data: vec![1, 2, 3] };
assert_eq!(6, other_foo.calls_foo_struct_func());
}
from irust.
the structs needs to contain each other, here is an example, I didnt check it ( I will later) but Im pretty sure it should trigger a borrow error
struct IRust {
cursor: Cursor
}
struct Cursor {}
impl Cursor {
fn dostuff(&self, irust: &IRust) {}
}
impl IRust {
fn somefn(&self) {
self.cursor.dostuff(&self) //borrow error here ?
}
from irust.
btw, I meant self referential struct (struct that contains elements that refrence itself)
from irust.
Nope, no borrow errors (just tested it). Note that you can change the self.cursor.dostuff(&self)
line to self.cursor.dostuff(self)
, and AFAIK it stays the same in functionality. The reference is given to a different scope (the dostuff
function above) which drops the ref after the function ends, and so nothing bad happens. In fact, you could probably have a reference to IRust
in the Cursor
struct and it would work (of course you would need lifetime annotations though AFAIK).
Note that if IRust
was defined like this, things would be a different story (this wouldn't work):
struct IRust<'r> {
irust: &'r IRust,
}
from irust.
Ok that's great! so we'll just refactor the whole code base with you're idea, hopefully with it we can remove most impl block and delegate functions to independent structs
from irust.
@sigmaSd sure, but I’m not quite finished. I’ve finished much of the general refactor, but because I’ve moved the TerminalCursor
(which doesn’t implement the Clone
trait) into the Cursor
struct, I’m not sure how to implement Clone
for Cursor
. Any ideas (on copying/cloning crossterm’s TerminalCursor
)? (I’m sure you know this, but as a reminder cloning is used to save and reset the cursor AFAIK.)
I'll go ahead and open a PR with my changes where we can figure things out.
from irust.
I got off on a bit of rabbit trail with #44 xD (when I started merging Printer
and writer.rs, I wanted to use Cursor
in a way that needed that refactor, see end of this comment). I’ll get working on point 2 today if all goes well.
@sigmaSd What do you think of making the Printer
struct contain the Cursor
, and handle moving it and such? To me it makes sense to have the printer control the cursor.
from irust.
@smolck #47 is almost done, its probably safe now if you want to start to work on anything you want ^^
Btw for writer and printer, I think we should merge them, change Printer struct to Writer struct and replace all print words with write words to match crossterm terminology and give people who reads the code less headache xD
from irust.
Ok, cool! Thanks for the info! Hopefully I can get a PR for the changes relatively soon.
from irust.
I refactored here #74 Printer is finally isolated and testable.
I think that the current abstraction level is nice.
Hopefully this will allow for easier testing and feature addition.
from irust.
Related Issues (20)
- Add :cargo command
- Add predefined themes
- Rework how irust server works
- Use `rustyline` or `reedline` for input? HOT 1
- Move theme settings from config.toml to themes config
- Improve Jupyter kernel support HOT 7
- typing twice in windows with crossterm-0.26+ HOT 2
- Can irust repl evaluate code with release mode instead of debug mode? HOT 3
- Cannot use two `irust_repl`s at the same time. HOT 27
- Rust analyzer is not working on Windows HOT 6
- Interactive usage HOT 3
- Check statements with cargo check HOT 1
- how make irust works with nightly ? error: toolchain 'stable-x86_64-unknown-linux-gnu' is not installed HOT 2
- Using IRust for live scripting of a host HOT 8
- Custom keybindings? HOT 15
- [Feature Request]: Support for `proc_macro` APIs HOT 7
- [Feature Request]: Ability to load custom `Cargo.toml` HOT 2
- Async repl not working properly HOT 5
- irust v1.50.0 failed to compile HOT 2
- Use `rust-analyser` instead of `racer` for autocompletion HOT 7
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from irust.