-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Ensure users don't create a .wasp file #2418
base: main
Are you sure you want to change the base?
Conversation
acd4f7c
to
11862bc
Compare
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.
Hey @komyg, thanks for the effort!
You have the right idea, and I see you know how to use both Haskell and our codebase (props for using relfile
and IOUtil.doesFileExist
), so I have no doubt we'll be merging this soon.
There are some improvements we can do, I left three requests (the biggest of which is moving the logic into findWaspFile
).
Let me know if you have any questions :)
waspc/src/Wasp/Project/Analyze.hs
Outdated
waspDirExists :: Path' Abs (Dir WaspProjectDir) -> IO (Either String (Path' Abs (Dir WaspProjectDir))) | ||
waspDirExists waspDir = do |
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.
This function is called waspDirExists
, implying that it checks whether a .wasp
directory exists and returns either an error or a path to the directory.
But, in reality, the function does something else:
- If a
.wasp
directory does exist, it returns a path to the existing.wasp
directory (so far so good) - If the
.wasp
directory doesn't exist, it returns a path to the non-existing.wasp
directory (this is the first weird part) - If a
.wasp
file exists, it returns an error saying that.wasp
is a file (but the function's name implies it should only care about whether the.wasp
directory exists and report on that).
Imagine what happens when someone else tries to use this function in a different context (e.g., to really check whether the .wasp
directory exists). I think they'd end up very confused :)
Anyway, for this issue, we don't care whether the .wasp
directory exists or not. It can exist, it doesn't need to exist - it's all the same to us.
We only care about whether a .wasp
file exists, so we should write a function to check for that, and name it accordingly.
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.
Yes, you are right.
My initial idea here, was to make this function return the waspDir
as a right value (in case there was no .wasp file), or an error as a left value.
Then I would send the right result to the next function call: waspFilePathOrError <- left (: []) <$> findWaspFile waspDir
, because that function takes the waspDir
as an argument.
However, I didn't end up doing this, and in the end I should just have returned a boolean or empty right value.
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.
My original plan for this implementation was to create a sort of pipeline in which the result of the previous function would go into the next function, as it is usually done in railway oriented programming.
This way, I would be able to avoid having two case
statements in the analyzeWaspProject
, because the next waspFilePathOrError
would only be called if its input was a right value, otherwise it will pass through the left value unchanged.
Unfortunately, I was unable to do that, and I forgot to change the return type of this function to boolean
or an empty right value.
waspc/src/Wasp/Project/Analyze.hs
Outdated
let waspDotWaspPath = waspDir </> [relfile|.wasp|] | ||
isFile <- IOUtil.doesFileExist waspDotWaspPath | ||
if isFile | ||
then return $ Left "The path to the Wasp project is a file, but it should be a directory." |
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.
The "Wasp project" is the user's project, not the stuff in the .wasp
directory.
Also, users don't really care about the .wasp
directory, so this message gives out unnecessary information. If I read it, I'd think Wasp expects me to create a .wasp
directory (which isn't true).
We should tell users something that conveys the message "Hey, your .wasp
file can't be called .wasp
, it needs to have a proper name," no need to mention the .wasp
directory at all.
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.
Ok, I've re-phrased the message as: Invalid file name for the .wasp file. Please rename it to [something].wasp.. What do you think?
waspc/src/Wasp/Project/Analyze.hs
Outdated
dirResult <- waspDirExists waspDir | ||
case dirResult of | ||
Left err -> return (Left [err], []) | ||
Right _ -> do | ||
waspFilePathOrError <- left (: []) <$> findWaspFile waspDir |
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 better place to handle this check is the function findWaspFile
.
After all, we don't care about the directory at all. All we care about is:
- Do the users have a
*.wasp
file? - If they do, is it called something other than just
.wasp
? - If not, tell them to name it properly.
That's on me, the issue could have been explained better. Sorry for the troubles :)
So, to summarize one last time:
While looking for a
*.wasp
file, we should also ensure that the found file isn't called.wasp
While you're at it, you might also want to check whether there are multiple .wasp
files and report that.
Because, in theory, Wasp will only find one of *.wasp
files. Let's say we have two of them: something.wasp
and .wasp
. findWaspFile
could return the something.wasp
, letting .wasp
slip under the radar and cause problems later.
And even if it didn't, allowing multiple .wasp
files can be confusing.
9e3bada
to
9830797
Compare
Hey @sodic, thank you for your review, it was very through! I believe I addressed most of your comments. Could you review again, please? The only thing left is making sure we don't have more than one .wasp file. I am currently working on that, but it is not done yet. If I can get it done before your next review, then I can be a part of this PR, otherwise, I can make it into another PR. What do you think? |
Description
Fixes: #2406
Select what type of change this PR introduces:
Update Waspc ChangeLog and version if needed
If you did a bug fix, new feature, or breaking change, that affects waspc, make sure you satisfy the following:
Update example apps if needed
If you did code changes and added a new feature or modified an existing feature, make sure you satisfy the following:
waspc/examples/todoApp
as needed (updated modified feature or added new feature) and manually checked it works correctly.waspc/headless-test/examples/todoApp
and its e2e tests as needed (updated modified feature and its tests or added new feature and new tests for it).