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

Replacing fields results in an invalid tree #78

Open
kneasle opened this issue Jan 13, 2021 · 10 comments
Open

Replacing fields results in an invalid tree #78

kneasle opened this issue Jan 13, 2021 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@kneasle
Copy link
Owner

kneasle commented Jan 13, 2021

Suppose that the cursor is over a field in an object (cursor in <>):

{
     <"key": null>
}

If we type rt at this point will result in the following invalid object:

{
    <true>
}

AFAIK, this is not an easy fix (at least, no easy fix that actually solves the cause of the problem). The main issue is that objects implicitly create fields to contain their children, a very nice UX feature that causes some jank with tree validation.

@kneasle kneasle added the bug Something isn't working label Jan 13, 2021
kneasle added a commit that referenced this issue Jan 14, 2021
@stokhos
Copy link
Contributor

stokhos commented Jan 20, 2021

sapling/src/editor/dag.rs

Lines 412 to 415 in b5ec298

// Ideally, replacement should just be implemented as deletion followed by insertion (which
// would fix at least one existing bug with replacement in JSON objects), but that is not
// possible in the current architecture because nodes like Json::Field cannot have their
// children deleted.

I'm working on this one, and I'm not sure why do you think it is not feasible ?

children.push(arena.alloc(Json::Field([s, v])));

Was there a reason that you put an immutable array inside Json::Field?

Is this sort of related to #11

@kneasle
Copy link
Owner Author

kneasle commented Jan 20, 2021

I'm working on this one, and I'm not sure why do you think it is not feasible ?

Oh hang on - I'm also working on this 😅 - at least I'm doing some more huge refactoring, which will eventually fix this.

The thing that made me say this might be hard is that just fixing it for JSON objects is not really enough - I'd like to actually fix the problem that causes this. Unless I've missed something? - it's entirely possible that I'm making a mountain out of a molehill here.

Was there a reason that you put an immutable array inside Json::Field?

Yes - it's needed because Ast::children has to return a reference to a slice with the same lifetime as the node. And the only way to do this (without unsafe) is to explicitly store the key and value as a slice. The refactoring I'm doing is to change Ast into a Node type which has an AstClass and a Vec of children, and with that change there'll be no need to do janky things like that.

Is this sort of related to #11

Perhaps? I don't think it has that much connection.

@stokhos
Copy link
Contributor

stokhos commented Jan 21, 2021

The thing that made me say this might be hard is that just fixing it for JSON objects is not really enough - I'd like to actually fix the problem that causes this. Unless I've missed something? - it's entirely possible that I'm making a mountain out of a molehill here.

Looking forward to your refactoring. Kind of want to get a master degree in CS. This is so fun.

@kneasle kneasle self-assigned this Jan 21, 2021
@kneasle
Copy link
Owner Author

kneasle commented Jan 21, 2021

Kind of want to get a master degree in CS. This is so fun.

Go for it! I've no idea how the US university(/college?) system works, but it must be possible to do another degree. Fair warning, though - degree level CS is just as much about learning logic and maths as it is about programming - but knowing the theory is incredibly useful, even if learning it is less fun than just writing code all the time 😆.

@stokhos
Copy link
Contributor

stokhos commented Jan 21, 2021

Actually I'm close to get a master degree in math. I learned some algorithm, and data structure from math department, but I have almost forgotten all of them. I definitely will get a degree in CS in near future.

@kneasle
Copy link
Owner Author

kneasle commented Jan 25, 2021

Actually I'm close to get a master degree in math. I learned some algorithm, and data structure from math department, but I have almost forgotten all of them. I definitely will get a degree in CS in near future.

Oh cool! I also enjoyed the course we had on algorithms and datastructures - until I got to an exam and discovered that the exams were just really really hard 😆. But I think that was just the person who wrote the exam being overoptimistic about how good the students were.

@stokhos
Copy link
Contributor

stokhos commented Jan 25, 2021

But I think that was just the person who wrote the exam being overoptimistic about how good the students were.
Profs always have unrealistic high expectation on students

@kneasle
Copy link
Owner Author

kneasle commented Jan 31, 2021

Absolutely 😆! This wasn't the worst module, actually - we had one exam where one of the examiner's reports said something along the lines of 'even compared to other years, this group did badly'...

@stokhos
Copy link
Contributor

stokhos commented Feb 2, 2021

Absolutely laughing! This wasn't the worst module, actually - we had one exam where one of the examiner's reports said something along the lines of 'even compared to other years, this group did badly'...

Haha, actually, it was not you guys did badly in exam. It was the Prof didn't pay much attention in teaching.

@kneasle
Copy link
Owner Author

kneasle commented Feb 3, 2021

That's often true, I'll admit. But I think this was the opposite - the lecturer put so much effort into teaching that he expected the students to do really well in his crazily hard exam...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants