Skip to content

Conversation

@PoshAlpaca
Copy link

@PoshAlpaca PoshAlpaca commented Apr 26, 2022

Adds support for Vapor 4. Originally implemented by rabc. I've combined it with our custom filterGeographyDistanceWithin and sortByDistance functions.

@PoshAlpaca
Copy link
Author

@0xTim ready for review 🎉

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Great work! Few comments and things that can be updated. Some other things to look at are:

  • use async/await throughout the tests assuming it's available to us. Updating the manifest to 5.5 and macOS 12 will help
  • we don't have any CI any more, we should set up GH actions to run the tests to make sure things work

@PoshAlpaca
Copy link
Author

@0xTim Thanks for the feedback! Can you take a look again?

@PoshAlpaca
Copy link
Author

Also, I was thinking to extend DatabaseSchema.DataType with static properties so that fields in migrations can be declared like so:

.field("location", .geometricPoint2D)

instead of:

.field("location", GeometricPoint2D.dataType)

What do you think?

@0xTim
Copy link
Member

0xTim commented May 3, 2022

@PoshAlpaca yep I think that's a great idea to provide a cleaner API

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Looking great! Last couple of changes just to CI then we're good. We should have CI passing in this PR

And let's add the data type stuff as well if it's simple

@PoshAlpaca PoshAlpaca requested a review from 0xTim May 3, 2022 10:29
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Looks great! Might need to tweak the on section of CI to get it to run or wait until it's merged. Happy to merge however and fix in another PR if necessary

@PoshAlpaca PoshAlpaca merged commit ea14dfc into brokenhandsio:master May 3, 2022
@PoshAlpaca PoshAlpaca deleted the vapor4 branch May 3, 2022 13:32
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.

4 participants