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

Small fixes in the generator #2269

Merged
merged 12 commits into from
Sep 27, 2024
Merged

Small fixes in the generator #2269

merged 12 commits into from
Sep 27, 2024

Conversation

sodic
Copy link
Contributor

@sodic sodic commented Sep 6, 2024

Small stuff I noticed while working on #2202.

-- These are optional executor-specific JSON options we pass
-- directly through to the executor when submitting jobs.
data ExecutorOptions = ExecutorOptions
{ pgBoss :: Maybe JSON,
simple :: Maybe JSON
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use this as far as I can see, and it's not documented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Origin story:
Screenshot 2024-09-16 at 15 09 21

&& (length (toFilePath path) > length (".wasp" :: String))

hasExtension :: Path' s (File f) -> String -> Bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't like about this function is that it isnt' obvious if it takes .ext or ext as second argument, but it will not always work correctly in the latter case. Actually, if I had to guess, I would think it takes ext -> since that is extension, not .ext. I would either make so it exxpects ext + document that in a short comment, or remove the function and just inline this logic where it is used.

Another thing: you can go with (Path s) r (File f) as it doesn't matter if its Win or Posix path.

findWaspTsFile files = WaspTsFile . (waspDir </>) <$> find (`hasExtension` ".wasp.mts") files
findWaspLangFile files = WaspLangFile . (waspDir </>) <$> find isWaspLangFile files
isWaspLangFile path =
path `hasExtension` ".wasp"
&& (length (toFilePath path) > length (".wasp" :: String))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm this is an interesting line of code -> we are making sure we don't pick up .wasp dir as wasp file.
When I saw this, I felt like we should also do it for .wasp.mts. I know it is not likely that there will be .wasp.mts directory, but this way we are sure + we are consistent.

So what seems best to me now is to take this piece of logic that checks length and also add it to hasExtension, that because it really belongs there I think, and you also that way got it applied for both cases here + it is nicer to read.

Comment on lines 111 to 112
[ SP.fromAbsDir workingDir ++ "node_modules/wasp-ts-sdk/dist/run.js",
SP.fromAbsDir workingDir ++ ".wasp/config/main.wasp.mjs"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work on Win, because workingDir will have Win separators and you have posix separators on the right.
In general I recommend converting from StrongPath at the last moment possible, that will save you from mistakes like this.
So in this case:

SP.fromAbsFile $ workingDir </> [relfile|node_modules/wasp-ts-sdk/dist/run.js|],
...

case tscExitCode of
ExitFailure _status -> return $ Left ["Error while running TypeScript compiler on the *.wasp.mts file."]
ExitSuccess -> do
otherChan <- newChan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow otherChan, what happened here :D

otherChan
)
case runExitCode of
ExitFailure _status -> return $ Left ["Error while running the compiled *.wasp.mts file."]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use actual file name here. You have it available. And you avoid a bit of hardcoding.

)
case runExitCode of
ExitFailure _status -> return $ Left ["Error while running the compiled *.wasp.mts file."]
ExitSuccess -> return $ Right []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No decls? Aha I guess we don't yet have that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 127 to 130
-- let payload:: String = read $ T.unpack text
liftIO $ putStrLn "Ovo smo dobili na stdout"
-- parse payload as json
let json = Aeson.toJSON payload
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha this is still WIP! Sorry I thought this PR is ready for review since it wasn't draft PR.

Copy link
Contributor Author

@sodic sodic Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aw man, I made a git error.
The Analyze.hs file was never supposed to be here, I must have committed it by accident.

Sorry for wasting your time, my bad!

What worries me is that you got through the entire review without noticing that the code is too horrible for something I'd send to code review.

Commenting with a straight face at all the atrocities and only realizing something was off when you encountered a string in Croatian.

Is that what you think of me, Martin? I don't know whether to praise your patience or worry about your perception 😄

Anyway, I'll remove this file from the PR. Please take a look at the other files. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, I believed in you!

All right, I will take another look. Maybe of some the change syou did in this file though, and that I commented on, are still present in the version of the code you are woroking on? In that case do take a look at my comments on Analyze.hs and see if you can apply them.

Copy link
Member

@Martinsos Martinsos Sep 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe this is just your sneaky way of giving up on a bad PR :D? "ha it was a joke from the very start"

Comment on lines 185 to 188
data WaspFile
= WaspLangFile {_path :: !(Path' Abs File')}
| WaspTsFile {_path :: !(Path' Abs File')}
deriving (Show)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, feels a bit unusual. I get why you did it though, you want to be able to say it is one of them but you don't want to yet know which one. Records in sum types are usually an anti-pattern, but these records are identical except for the data constructor, so it works.

What if you want to specify somewhere that a function needs specifically path to WaspTS file? Or Sepcifically WaspLangFile? We have that knowledge but we didn't put it in StrongPath.

I guess soltuion would be

data WaspLangFile
data WaspTsFile
data WaspFile = WaspLang (Path' Abs WaspLangFile) | WaspTs (Path' Abs WaspTsFile)

And I wouldn't bother with _path, I probably won't be needing that anyway as I will most likely always want to deconstruct hm.
But ok, this is verbose a bit, and it is all quite localized so I guess it is not important. Although you do export WaspFile (..), which makes it less localized hm. Ok, I am ok with this what you have, although I would be tempted to go this extra step.

@Martinsos
Copy link
Member

Ha small fixes you say, but then there is also the flow for main.wasp.ts vs main.wasp.lang!

@@ -25,7 +25,7 @@ import Data.List.NonEmpty (NonEmpty, fromList)
-- The mechanism to catch errors is only there to assist in collecting more errors, not recover.
-- There may optionally be additional errors or non-fatal warnings logged in the State.
newtype Generator a = Generator
{ _runGenerator :: ExceptT GeneratorError (StateT GeneratorState Identity) a
{ _runGenerator :: ExceptT GeneratorError (State GeneratorState) a
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this just so I could ask what's the purpose of using StateT s Identity instead of State s).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, can't remember at the moment! I guess it is the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I know, it is.

@@ -25,7 +25,7 @@ import Data.List.NonEmpty (NonEmpty, fromList)
-- The mechanism to catch errors is only there to assist in collecting more errors, not recover.
-- There may optionally be additional errors or non-fatal warnings logged in the State.
newtype Generator a = Generator
{ _runGenerator :: ExceptT GeneratorError (StateT GeneratorState Identity) a
{ _runGenerator :: ExceptT GeneratorError (State GeneratorState) a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, can't remember at the moment! I guess it is the same thing?

@sodic sodic merged commit 0dbd797 into main Sep 27, 2024
6 checks passed
@sodic sodic deleted the filip-small-generator-fixes branch September 27, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants