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

Update example apps to account for recent changes #809

Merged
merged 87 commits into from
Nov 11, 2022

Conversation

sodic
Copy link
Contributor

@sodic sodic commented Nov 9, 2022

Updates the example apps with recent changes to the project structure and environment variable definitions. PRs covered by this:

sodic and others added 30 commits October 12, 2022 15:28
Co-authored-by: Shayne Czyzewski <[email protected]>
Co-authored-by: Martin Šošić <[email protected]>
Copy link
Contributor

@shayneczyzewski shayneczyzewski left a comment

Choose a reason for hiding this comment

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

Looking good @sodic! Tried running a few, all seems good. Nice work refactoring all of them. Left a few comments/questions 👍🏻 I wonder if, given your updates here, if in the ChangeLog update describing this we can give a TL;DR update of what they can do to refactor their apps? I know it is pretty straightforward but may help some if we list it out.

@@ -40,7 +40,7 @@ watch waspProjectDir outDir ongoingCompilationResultMVar = FSN.withManager $ \mg
currentTime <- getCurrentTime
chan <- newChan
_ <- FSN.watchDirChan mgr (SP.fromAbsDir waspProjectDir) eventFilter chan
let watchProjectSubdirTree path = FSN.watchDirChan mgr (SP.fromAbsDir $ waspProjectDir </> path) eventFilter chan
let watchProjectSubdirTree path = FSN.watchTreeChan mgr (SP.fromAbsDir $ waspProjectDir </> path) eventFilter chan
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change supposed to be in this PR?

waspc/run Outdated
@@ -24,7 +24,7 @@ TEST_CMD="cabal test"
TEST_UNIT_CMD="cabal test waspc-test"
TEST_CLI_CMD="cabal test cli-test"
TEST_E2E_CMD="cabal test e2e-test"
TEST_E2E_DELETE_GOLDENS="rm -r $PROJECT_ROOT/e2e-test/test-outputs/*-golden"
TEST_E2E_DELETE_GOLDENS="rm -rf $PROJECT_ROOT/e2e-test/test-outputs/*-golden"
Copy link
Contributor

@shayneczyzewski shayneczyzewski Nov 9, 2022

Choose a reason for hiding this comment

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

This change may be old (i.e. already merged) now too.

let newContents = beforeMatch ++ matchBeforeVersion ++ newVersion ++ afterMatch
return (newContents, oldVersion)
let (beforeMatch, match, afterMatch, submatches) =
cabalFileContents TR.=~ ("^(version: *)([0-9]+\\.[0-9]+\\.[0-9]+)" :: String) :: (String, String, String, [String])
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like on this line we lost an \\.[0-9]+ at the end. Was that intended?

@@ -0,0 +1 @@
DATABASE_URL=postgresql://postgres:devpass1234@localhost:5432/thoughts-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to remove this file entirely since it is .gitignore'd and the only env file we have committed for examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, thought so too. I can remove it.

@sodic
Copy link
Contributor Author

sodic commented Nov 9, 2022

@shayneczyzewski Thanks for the review, three of your comments should be resolved now, I forgot to push a base branch merge (as you guessed, these changes are already in main).

I wonder if, given your updates here, if in the ChangeLog update describing this we can give a TL;DR update of what they can do to refactor their apps? I know it is pretty straightforward but may help some if we list it out.

Good idea, I'll do that!

@@ -1,5 +0,0 @@
-- CreateTable
CREATE TABLE "Excuse" (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was causing DB errors. We have two init migrations, this was the one that was out of date. I kept the other one.

@sodic
Copy link
Contributor Author

sodic commented Nov 11, 2022

@shayneczyzewski Addressed all your comments and added the migration guide to the changelog. I think this is it.

Note: I didn't update this page: https://wasp-lang.dev/blog/2021/03/02/wasp-alpha. It's pretty old and hasn't been updated in ages (e.g., it uses Wasp syntax I've never seen). Everything else should be taken care of.

Copy link
Contributor

@shayneczyzewski shayneczyzewski left a comment

Choose a reason for hiding this comment

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

Awesome job @sodic! Found one minor typo but all this looks great. Love the Changelog update! 🚀 great job getting all these examples up to date 👍🏻

waspc/ChangeLog.md Outdated Show resolved Hide resolved
```
Do this for all external imports in your `.wasp` file. After you're done, there shouldn't be any occurences of the string `"@ext"`.

That's it! You should now have a fully working Wasp project in the `foo` directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome guide!!! 🥳

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I accumulated a lot of experience with this in recent days :)

Base automatically changed from filip-update-docs-new-structure to main November 11, 2022 17:36
@sodic sodic merged commit 2fe7398 into main Nov 11, 2022
@shayneczyzewski shayneczyzewski mentioned this pull request Nov 14, 2022
5 tasks
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.

2 participants