stringify test cases and some comments #122
Conversation
jaycarlton
changed the title
There was a problem hiding this comment.
Hi @jaycarlton , thanks for all the work put in here and very sorry for my late reply :'(.
I think our best course of action is to decline this PR and create some minor ones. I am happy to have a call with you to align our priorities.
| @@ -149,16 +149,17 @@ impl Lexer { | |||
|
|
|||
| /// Checks the next token without advancing position | |||
| /// See also [advance_token](Self::advance_token) | |||
| /// TODO: is there not a way to do this without mutating `self`? | |||
There was a problem hiding this comment.
This kind of questions shouldn't be in code. The reason is that peek_token advances te token position. Otherwise we would hace to parse it all over again if we "accept" it
| pub fn peek_token(&mut self) -> TokenType { | ||
| let position = self.position; | ||
| let orig_position = self.position; |
There was a problem hiding this comment.
Please try not to rename variables unless there is a need for it or a previous discussion.
| tk | ||
| } | ||
|
|
||
| /// Advances position. This is used in conjunction with [`peek_token`](Self::peek_token) | ||
| /// It is a noop if the has not been a previous peek_token | ||
| /// It is a noop if there has not been a previous peek_token |
There was a problem hiding this comment.
Thanks!
annoying but, could you do this is a separate PR?
| @@ -446,6 +447,7 @@ impl Lexer { | |||
|
|
|||
| // Private methods | |||
|
|
|||
| // Move position to the end of input and return a `LexerError` value. | |||
There was a problem hiding this comment.
Let's discuss how to best comment code first. This is an awesome contribution but a bit more difficult than it looks like.
Let's do that is separate PRs please.
| @@ -10,6 +10,7 @@ use crate::expressions::{ | |||
| types::ParsedReference, | |||
| }; | |||
|
|
|||
| // TODO: why not jsut pass in a `LexerMode`? It's more expressive than a bool arg. | |||
There was a problem hiding this comment.
What do you mean by that? Maybe we can discuss?
| @@ -464,6 +474,7 @@ impl Parser { | |||
| Some(name) => self.get_sheet_index_by_name(name), | |||
| None => self.get_sheet_index_by_name(&context.sheet), | |||
| }; | |||
| // todo: bust out helper function for this calculation | |||
There was a problem hiding this comment.
I don't know what you mean by this. Maybe let's talk?
| @@ -30,6 +30,7 @@ pub struct CellReference { | |||
| #[derive(Clone)] | |||
| pub struct CellReferenceRC { | |||
| pub sheet: String, | |||
| // 1-based index | |||
There was a problem hiding this comment.
Again, rows and columns are always 1-based and sheets 0-based. I think we should only document it once.
| @@ -369,9 +369,11 @@ pub struct Font { | |||
| #[serde(default = "default_as_false")] | |||
| #[serde(skip_serializing_if = "is_false")] | |||
| pub u: bool, // seems that Excel supports a bit more - double underline / account underline etc. | |||
There was a problem hiding this comment.
Correct, you could open a ticket for this. And actually this might be a nice first issue.
| @@ -18,8 +18,8 @@ export function updateNameSelectedWorkbook(model: Model, newName: string) { | |||
| console.warn("Failed saving new name"); | |||
| } | |||
| } | |||
| const modeBytes = model.toBytes(); | |||
| localStorage.setItem(uuid, bytesToBase64(modeBytes)); | |||
| const modelBytes = model.toBytes(); | |||
There was a problem hiding this comment.
ooops, thanks! Care to make a PR just for this?
| @@ -0,0 +1,146 @@ | |||
| use super::Parser; | |||
There was a problem hiding this comment.
Hi @jaycarlton , these tests are amazing? But why did you add them? Did you add them because you want to learn how to use the API or because you are covering some case that wasn't covered before. If it is the second case we definitely need them in!
Added a few test cases to help learn bits of the system. Added some comments while exploring the code.