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.

Sign up for GitHub

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

Jump to bottom

Add c:JSONAPI.View.get_field/3 #273

Merged
merged 4 commits into from
Dec 28, 2022
Merged

Add c:JSONAPI.View.get_field/3 #273

merged 4 commits into from
Dec 28, 2022

Conversation

whatyouhide
Copy link
Contributor

Closes #272.

@whatyouhide whatyouhide requested a review from a team as a code owner September 20, 2022 19:22
@whatyouhide whatyouhide mentioned this pull request Sep 20, 2022
Closed
mattpolzin
mattpolzin reviewed Sep 20, 2022
View reviewed changes
Copy link
Member

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

Just have some ideas around the test.

test/jsonapi/view_test.exs Outdated Show resolved Hide resolved
test/jsonapi/view_test.exs Outdated Show resolved Hide resolved
mattpolzin
mattpolzin approved these changes Sep 21, 2022
View reviewed changes
Copy link
Member

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

Nice!

My approval isn't official since I am not a maintainer, but this looks good to me once tests pass (only maintainers can trigger the CI tests to run).

@whatyouhide
Copy link
Contributor Author

Ping @davydog187 and/or @doomspork? 🙃

@mattpolzin
Copy link
Member

@doomspork Would you be interested in having me help maintain (review and/or release) this project? I'm very interesting in chipping in more formally and keeping momentum up. Sorry, I feel a comment on a PR is a terrible place to make that plea, I've just had trouble finding a better way to reach out!

@mattpolzin
Copy link
Member

@whatyouhide do you mind merging the main branch into yours again to see if GitHub will recognize that it hasn't run CI against this branch yet? I now have the ability to merge this PR once we figure out why GitHub has forgotten that there are CI actions to run against it!

@doomspork
Copy link
Member

@mattpolzin you'll need to re-approve as well 👍

mattpolzin
mattpolzin previously approved these changes Dec 27, 2022
View reviewed changes
Copy link
Member

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

Re-approving (pending us getting CI to run).

@whatyouhide
Copy link
Contributor Author

@mattpolzin just rebased on master and pushed.

@mattpolzin
Copy link
Member

Thank you! I am fixing CI over here so I'll have you merge once more after that lands. Appreciate your patience!

@mattpolzin
Copy link
Member

Ok, that's sorted when you get the chance to merge master in again @whatyouhide .

@davydog187
Copy link
Member

Sorry all, just catching up here. Let me know if I can help @whatyouhide @mattpolzin

@doomspork
Copy link
Member

Updated the branch protections which should now allow us to update a branch to master 👍 Just did so here and it has kicked off CI.

@doomspork doomspork merged commit 8fe87f6 into beam-community:master Dec 28, 2022
@whatyouhide whatyouhide deleted the get-field branch December 29, 2022 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doomspork doomspork doomspork approved these changes

@mattpolzin mattpolzin mattpolzin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dynamic fields in views
4 participants
@whatyouhide @mattpolzin @doomspork @davydog187