Closed BuggusMageevers closed 1 year ago
@marcusziade @MatsMoll could you guys take a look to this ? :)
I updated the tests and tokens to include coverage of the remaining tokens. That brings the project up to section 1.4 of the book.
I didn't add a peek
function at this time as it was only two cases which needed it. I don't feel it's worth it to add some lines to take away 2. Here are some peek()
examples:
Getting next index, no endIndex
checking:
func peekIndex() -> String.Index {
let peekIndex = input.index(after: currentIndex)
}
Getting next index with endIndex
checking:
enum Result {
case success(String.Index)
case failure(Token)
}
func peek() -> Result {
let peekIndex = input.index(after: currentIndex)
guard peekIndex != input.endIndex else { return .failure(.eof) }
return .success(peekIndex)
}
Getting the next Character
with endIndex
checking:
enum Result {
case success(Character)
case failure(Token)
}
func peek() -> Result {
let peekIndex = input.index(after: currentIndex)
guard peekIndex != input.endIndex else { return .failure(.eof) }
return .success(input[peekIndex])
}
I think these may be useful later if additional refactoring increases the uses cases for this function. At this time, I think it's simpler to leave checking for ==
and !=
within the switch. I'm interested to hear your thoughts.
I added an initializer which allows for removal of both lazy
on currentIndex
and mutating
on currentChar
s getter
. I do still feel this avoids embracing the simplicity of Swift through use of default initializers and lazy vars. However, I do not feel that strongly so I added an initializer.
An extension is not necessary to add initializers. An extension can be helpful to organize the code, but in that case, placing the properties and initializers in the case definition and have the nextToken
function in an extension. Again, though, this is style and not a limit of the language. Would you like to have the initializer in an extension instead?
I added a dictionary for keywords as well. Creating an extension on Dictionary to produce a computed property for the default values I find a little strange. Is there a reason you chose to implement default keywords in this way? Keywords have nothing to do with dictionary so adding them to Dictionary as an extension, even a constrained extension, doesn't feel right. Perhaps added a function to Token which returns a dictionary of the default tokens instead? What do you think?
I really appreciate the feedback. I've not had the opportunity to work with someone on a swift project before this. Just my own hobby projects. This has been fun!
I added an a static var to Token with the default keywords. What do you think?
Looks very nice!
And regarding the Dictionary
, the reason for creating an extension on that one is so we can "look up" default values on the object it self.
Therefore, if we have a function signature similar to func doSomething(value: String, dict: [String: Token]) -> Token
.
When calling it we can look up the value using the dot syntax. doSomething(value: "something", dict: .defaultValue)
.
Therefore, behaving similar to the "shorthand" enums syntax.
Also, I did not see the peek()
stuff, I like that you went for a Rust like syntax with the Result
enum.
Also a bit nit picky, but I will not hold back the PR because of this.
What I ment with the "dumb" init in case if it was not clear.
I like when we can inject all variables in the struct as we like.
Therefore, having one init with the following arguments Lexer(input: <String>, currentIndex: <String.Index>, keyWords: Dictionary<String, Token>)
, ofc, we can still add defaults here. Therefore, I often use the autogenerated init by Swift, which I find it nice to have the freedom to set them as wish.
Furthermore, then adding another one for containing logical methods.
That is why I suggested the init in the extension, as this will create two different inits, where one needs to define the currentIndex
and another sets it to the starting index.
or use a static function as the following
e.g: Lexer.fromBeginning(input: <String>)
where currentIndex = input.startIndex
But ofc, this is my preference 😄
Thank you, @MatsMoll! 😊
I understand your reasoning behind the inits and agree it is your style.
I hadn't added the peek()
functions because it didn't really save that much space, but I offered some ideas for the two current use cases in nextToken()
. I'm glad you liked the rust-style Return
type. If you want peek()
added, we can get that into the next PR.
Speaking of which, I know you already closed your REPL PR, but I'd like to continue working with you any anyone else who wants to join in to complete the Swift implementation. Are you going to reopen the PR or open a new one? I've been busy with work, so I haven't started an REPL implementation yet. May later tonight depending on some other work things I have to finish.
Thank you, again. I look forward to working on the next segment together.
Awesome, let's work together on this @BuggusMageevers!
We will se when I get time to play around with this, but I will let you know if I setup a new PR with the updated REPL.
Btw, I am am MatsMoll in the discord, and I will most likely answer faster there
input
in the argument list.