-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Ellie Link Generator - v1 (Stateless & Messageless Files Only) #64
base: master
Are you sure you want to change the base?
Conversation
- ASSUMES version (tag_name) doesn't have a 'v' in it - I will have to make some changes for the Router subscriptions to work Is fetching it in JS worth it? I will have to make changes for Router to support subscriptions I will also make another version next commit with Router doing the HTTP request
I prefer this way unless supermacro prefers the JS version. I also have to start thinking of how to send the version to the Component
File server not working though, maybe I need to fetch upstream
- can now completely do "Stateless and Message-less" files
- Just needed to add more newline characters lol
I'm still reviewing your PR, but I had a thought pop into my head that I wanted to write down so I didn't forget: Ellie link generation should only run once for each file and henceforth the output should be saved in memory (in each page's
Do you get what I mean? |
@@ -24,6 +24,7 @@ type alias Model = | |||
, descriptionExample : Container.Model () Never | |||
, typeExample : Container.Model () Never | |||
, closeableExample : Container.Model CloseableExample.Model CloseableExample.Msg | |||
, version : String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this field be a Maybe String
?
Also, inside of Utils.elm
, you could refactor the definition of DocumentationRoute
such that the model
type variable becomes { a | version : Maybe String }
:
type alias Versioned a =
{ a | version : String }
type alias DocumentationRoute { a | version : Maybe String } msg =
{ title : RouteTitle
, update : msg -> Versioned model -> ( Versioned model, Cmd msg )
, category : ComponentCategory
, view : Versioned model -> Styled.Html msg
, initialModel : Versioned model
, saveExampleSourceCodeToModel : List SourceCode -> msg
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since an API request can fail, I think a Maybe String
will definitely be a better choice. I just went with strings for simplicity, and since I was hardcoding it anyway lol.
I'll change the versioning to Maybe String
, then introduce the extensible records in another commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't thinking about API requests failing, but rather that the version
environment variable won't exist for local development (at least, not by default??).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the versioning issue! Interestingly, I get errors when I try to declare Versioned
in the DocumentationRoute
type alias. I'll leave this unresolved in case this is a problem, though I think it should be fine.
-- current state
type alias Versioned a =
{ a | version : String }
type alias DocumentationRoute model msg =
{ title : RouteTitle
, update : msg -> Versioned model -> ( Versioned model, Cmd msg )
, category : ComponentCategory
, view : Versioned model -> Styled.Html msg
, initialModel : Maybe String -> Versioned model
, saveExampleSourceCodeToModel : List SourceCode -> msg
}
I wasn't thinking about API requests failing, but rather that the version environment variable won't exist for local development (at least, not by default??).
Oh that's a good point. Would that mean the URL generator for local development wouldn't work? I guess that would be a later issue, though.
showcase/src/elm/UrlGenerator.elm
Outdated
------------------- | ||
|
||
|
||
elliefy : String -> { title : String, code : String } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than returning non-sensical values such as { title = "Parsing Failure", code = ""Error! ..." }
, wouldn't it make more sense for this fn to return a Result
? Or am I overcomplicating this? Maybe there's some context I'm missing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely should have left a comment regarding this haha. Returning a Result
would probably be an end goal, but I just wanted to have a way to quickly see the errors if something went wrong for debugging, since this isn't really production ready.
If the function returned a result, we would have to find a way to somehow view it in the HTML page, and that'll get a bit annoying. Maybe Debug.log
would have been a better strategy though, but I think Debug
functions break elm optimizations.
What do you think? If we returned a result, the Container
module wouldn't really have a way to use the error, other than logging it. For production, though, if we write enough tests, I don't think it would matter too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that you could change the type signature of elliefy
to return a result, and then you would pipe the output to Result.withDefault
.
{ title, code } =
FileParser.elliefy elmCode
|> Result.withDefault { title = "parse error", code = "..." }
That way the type signature can already be something more appropriate.
we would have to find a way to somehow view it in the HTML page
On Ellie you mean?
Re: usage of Debug
:
Debug
calls aren't allowed for production builds if I'm not mistaken, but I'm ok committing Debug.log
calls if this also works for you. I would actually prefer going with this approach rather than returning a non-sensical record.
And once it's ready to deploy, we can remove the Debug.log
calls.
If we returned a result, the Container module wouldn't really have a way to use the error, other than logging it.
That's ok. If there's an error, then I think it's ok to make clicking on the ellie link icon a noop.
An improvement for the future would be to emit a Cmd msg
to log errors to https://sentry.io or some other error tracking service whenever parsing fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Ellie you mean?
I meant on the showcase, but you answered that question when you said "it's ok to make clicking on the ellie link icon a noop".
So I changed up the type signatures a bit more, and they should make a lot more sense.
As of now both FileParser.elliefy
and EllieLinks.fromSourceCode
both return Result String String
, with the error type just being a stringified version of the parser DeadEnds
type. Maybe we can define our own error type once the file parser gets more advanced?
EllieLinks.fromSourceCode
is currently the one that Debug.log
s the error (temporary).
Container.setSourceCode
takes care of this and transforms the Result String String
to a Maybe String
that's used in the component pages. This should mean any parsing errors would just result in the button being unclickable.
showcase/src/elm/UrlGenerator.elm
Outdated
|
||
|
||
toString : File -> String | ||
toString file = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few moving parts in this module, and in the context of maintainability, I think it would be nice to test more than just fromSourceCode
which is currently the only exposed fn in this Module.
There are 3 kinds of functions / code I see in this module:
- URL generation
- Anything to do with parsing and creating a
File
- Modifying a
File
to adhere to the Ellie API
Thus, I think you could actually have a second module called Utils.FileParser
which houses all the File
related logic.
Here you would expose some abstraction over Parser.run fileParser srcCode
.
-- something like this maybe?
parseFile : String -> Result ParseError File
parseFile =
Parser.run fileParser
Then a test or 2 could be written for parseFile
.
As for the modification code, I'm not sure where I stand on whether that code should be moved to another module or not right now. Might be overkill at this moment. But if it gets more complex in the future, then it would be justified to move it into its own module and add tests for this code too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate module just for the file parsing would be nice, especially since the file parser is not a trivial function. I've separated the UrlGenerator.elm
into EllieLinks
and FileParser
.
I think we can keep the modification code in the file parser code right now, but I can see it getting pretty complex. Right now, I don't think we need to move it.
I didn't make much changes to the functions in the FileParser.elm
file, so it still has the elliefy
as the export. I'll get around to fixing this later, so I added a new action item to the PR.
Thanks for such a detailed code review! Before I fix the issues, I'll just reply to the comment you made earlier:
I totally get what you mean, but the good news is that I think this already works that way! The URL Generator only executes every time the |
- encodeUrl -> encodeUrlComponent - `htmlCode` moved to bottom - `elliefyExports` is cleaner
Hey, just wanted to thank you again for the great code review. I won't be able to resolve all of them today, but will do them tomorrow or over the weekend. This project is super exciting! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! I wrote a few minor observations / nit-picks.
setSourceCode : List SourceCode -> Model m msg -> Model m msg | ||
setSourceCode sourceCodeList model = | ||
setSourceCode : Maybe String -> List SourceCode -> Model m msg -> Model m msg | ||
setSourceCode elmAntdVersion sourceCodeList model = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to setSourceCodeAndEllieLink
showcase/src/elm/UI/Container.elm
Outdated
ellieLink = | ||
case ( maybeSourceCode, elmAntdVersion ) of | ||
( Just elmCode, Just version ) -> | ||
Just <| EllieLinks.fromSourceCode version elmCode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh so nice and self-documenting 😊
let | ||
maybeSourceCode = | ||
sourceCodeList | ||
|> List.filter (\{ fileName } -> fileName == model.fileName) | ||
|> List.head | ||
|> Maybe.map .fileContents | ||
|
||
ellieLink = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ellieLink = | |
maybeEllieLink = |
Just to be consistent with the above maybeSourceCode
variable name.
[ ( "cursor", "pointer" ) ] | ||
|
||
else | ||
[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[] | |
[ ( "cursor", "not-allowed" ) ] |
|
||
|
||
iconContainer : IconContainerOptions msg -> Styled.Html msg | ||
iconContainer { icon, tooltipText, event, extraStyles } = | ||
iconContainer options = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iconContainer options = | |
iconContainer iconContainerOptions = |
-- literally parses the big chonk of code | ||
-- we don't need to manipulate imports so there's no need | ||
|
||
|
||
imports : Parser String | ||
imports = | ||
Parser.chompUntil "\n\n\n" | ||
|> Parser.getChompedString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-- literally parses the big chonk of code | |
-- we don't need to manipulate imports so there's no need | |
imports : Parser String | |
imports = | |
Parser.chompUntil "\n\n\n" | |
|> Parser.getChompedString | |
{-| literally parses the big chunk of code | |
we don't need to manipulate imports so there's no need | |
-} | |
importsParser : Parser String | |
importsParser = | |
Parser.chompUntil "\n\n\n" | |
|> Parser.getChompedString |
codeBlocks = | ||
let | ||
importsHelper : List CodeBlock -> Parser (Step (List CodeBlock) (List CodeBlock)) | ||
importsHelper revImports = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revImports
is a reversed list of imports (i.e. starting from bottom and ending at top)?
A comment here would help.
moduleDeclarationParser : Parser ModuleDeclaration | ||
moduleDeclarationParser = | ||
let | ||
moduleName : Parser String | ||
moduleName = | ||
Parser.chompUntil " " | ||
|> Parser.getChompedString | ||
in | ||
Parser.succeed ModuleDeclaration | ||
|. Parser.keyword "module" | ||
|. Parser.spaces | ||
|= moduleName | ||
|. Parser.spaces | ||
|. Parser.keyword "exposing" | ||
|. Parser.spaces | ||
|= exposes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General observation:
You have two naming conventions:
<parserName>Parser
<parserName>
- i.e. you don't explicitly say that it's a parser
Not a big deal since these functions aren't exposed, but it's an inconsistency nonetheless :)
-- copy and pasting YuichiMorita's PR since the elm/parser library hasn't implemented this yet :(( | ||
|
||
|
||
deadEndsToString : List DeadEnd -> String | ||
deadEndsToString deadEnds = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-- copy and pasting YuichiMorita's PR since the elm/parser library hasn't implemented this yet :(( | |
deadEndsToString : List DeadEnd -> String | |
deadEndsToString deadEnds = | |
{-| copy and pasting YuichiMorita's PR since the elm/parser library hasn't implemented this yet :(( | |
https://github.com/elm/parser/pull/38 | |
-} | |
deadEndsToString : List DeadEnd -> String | |
deadEndsToString deadEnds = |
_ -> | ||
"bruh moment" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hahaha temporary I assume?
Looks like there's an unrelated issue going on with the |
Hey @joshuanianji, Just want to check in. Any idea on when you'll have some time to continue working on this PR? |
Hey @supermacro! I've been really busy lately, but the term is finishing up soon and I hope to continue working on this PR within the next couple of weeks. Sorry for the wait! |
Still work to do for error handling to be better But now the type signature makes more sense
Container.setSourceCode fixed to accomodate this change
Hey @supermacro, I'll be able to work on this PR more often now that my term is almost over. Excited to see this project move forward! |
Hey @joshuanianji 👋 Hope you're doing well. I just wanted to check in and see if you have any plans on continuing this work. Regards, Gio |
Description
This PR showcases basic functionality for the Ellie Link Generator. It generates a simple AST of the file which is able to be manipulated to work in Ellie. It currently works on Stateless and Messageless files, though isn't able to inject the
Ant.Css
stylesheet into the code yet.By the way, ignore the first two commits where I was toying around with HTTP requests in JS and Elm.
Additional technical notes can be found on this comment.
Screenshots
(not applicable)
Action Items
defaultStyles
for ellie code demos to have correct styling appliedUtils.FileParser
.Container.elm
for more readability.Post-Merge