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 NameIDFormat #21

Merged
merged 2 commits into from
Oct 27, 2022
Merged

Add NameIDFormat #21

merged 2 commits into from
Oct 27, 2022

Conversation

fumieval
Copy link
Contributor

As mentioned in #20, I implemented a datatype for NameIDFormat, and updated the definition of Subject.

TODO: rebase after #20 is merged, or vice versa

@fumieval fumieval marked this pull request as ready for review October 11, 2022 05:18
mbg
mbg reviewed Oct 12, 2022
View reviewed changes
Copy link
Owner

@mbg mbg left a comment

Choose a reason for hiding this comment

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

This looks fine, thank you! There are the usual comments about style (mainly the comments following the constructors, rather than preceding them here; and the export list) and documentation (missing for the module and parseNameIDFormat).

wai-saml2.cabal Show resolved Hide resolved
@fumieval fumieval force-pushed the NameIDFormat branch 2 times, most recently from 2cd3f96 to 330efe3 Compare October 13, 2022 10:29
@fumieval
Copy link
Contributor Author

Updated the code/comment style

@fumieval
Copy link
Contributor Author

@mbg Friendly reminder that this is ready for review

@mbg mbg self-requested a review October 24, 2022 18:47
mbg
mbg reviewed Oct 24, 2022
View reviewed changes
Copy link
Owner

@mbg mbg left a comment

Choose a reason for hiding this comment

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

Thanks for documenting everything! There are some small formatting issues that need fixing.

I have also improved the CI a bit in #24 so that the Haddock documentation is now generated there and uploaded as build artifacts that can be inspected more easily.

src/Network/Wai/SAML2/NameIDFormat.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/NameIDFormat.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/NameIDFormat.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/NameIDFormat.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/NameIDFormat.hs Outdated Show resolved Hide resolved
@fumieval
Copy link
Contributor Author

Thank you for reviewing. I added a Haddock comment for the module

@mbg
Copy link
Owner

mbg commented Oct 26, 2022

Thanks for that! You marked all the other review comments as resolved, but I don't actually see those changes in the code?

@fumieval
Copy link
Contributor Author

Thanks for that! You marked all the other review comments as resolved, but I don't actually see those changes in the code?

Ah sorry, I accidentally force-pushed over the commited suggestion. Fixed now

mbg
mbg approved these changes Oct 27, 2022
View reviewed changes
Copy link
Owner

@mbg mbg left a comment

Choose a reason for hiding this comment

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

Thanks, that looks fine now!

@mbg mbg merged commit 30816a5 into mbg:master Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbg mbg mbg approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@fumieval @mbg