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

Feature: Discard nil value field #425

Merged
merged 10 commits into from
Jul 11, 2024
Merged

Feature: Discard nil value field #425

merged 10 commits into from
Jul 11, 2024

Conversation

jamesst20
Copy link
Contributor

@jamesst20 jamesst20 commented Jun 13, 2024 •
edited
Loading

Closes #424

Checklist:

  • [OK] I have updated the necessary documentation
  • [OK] I have signed off all my commits as required by DCO
  • [OK] My build is green

@jamesst20
Feature: Discard nil value field
3cb4fde 
Signed-off-by: James St-Pierre <[email protected]>
@jamesst20 jamesst20 requested review from ritikesh and a team as code owners June 13, 2024 14:52
@jamesst20 jamesst20 mentioned this pull request Jun 13, 2024
Closed
1 task
lessthanjacob
lessthanjacob reviewed Jun 17, 2024
View reviewed changes
Copy link
Contributor

@lessthanjacob lessthanjacob left a comment

Choose a reason for hiding this comment

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

Thank you for opening this and helping add some additional configurability to Blueprinter!

Most of my comments are primarily around naming, but happy to discuss further!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/blueprinter/helpers/base_helpers.rb Outdated Show resolved Hide resolved
spec/integrations/shared/base_render_examples.rb Outdated Show resolved Hide resolved
spec/integrations/shared/base_render_examples.rb Outdated Show resolved Hide resolved
spec/integrations/shared/base_render_examples.rb Outdated Show resolved Hide resolved
@jamesst20 jamesst20 force-pushed the discard_nil_field branch from d7093eb to 1a70e3e Compare June 18, 2024 15:45
@jamesst20
Copy link
Contributor Author

Thank you for opening this and helping add some additional configurability to Blueprinter!

Most of my comments are primarily around naming, but happy to discuss further!

Thanks for the quick review! Very much appreciated. I have applied all of your recommandations, after all I don't know much about this project conventions so I will stick to yours :)

@jamesst20 @lessthanjacob
Update spec/integrations/base_spec.rb
9e77b98 
Co-authored-by: Jake Sheehy <[email protected]>
Signed-off-by: James St-Pierre <[email protected]>
lessthanjacob
lessthanjacob previously approved these changes Jul 1, 2024
View reviewed changes
@jamesst20
Copy link
Contributor Author

Hey @lessthanjacob

May I kindly bump this and ask what's the next step now that is has your approval?

@lessthanjacob
Copy link
Contributor

Hey @jamesst20! I believe I may have suggested some changes to the tests that weren't totally valid syntax-wise. Do you mind addressing those locally and pushing up a fix? Apologies for that!

@jamesst20
Copy link
Contributor Author

jamesst20 commented Jul 10, 2024 •
edited
Loading

@lessthanjacob It's done! :) Indeed there was an extra , at the end of a line

Capture d’écran, le 2024-07-10 à 11 05 26

@lessthanjacob lessthanjacob merged commit c19d1dd into procore-oss:main Jul 11, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lessthanjacob lessthanjacob lessthanjacob approved these changes

@ritikesh ritikesh Awaiting requested review from ritikesh ritikesh is a code owner

Assignees

@jamesst20 jamesst20

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[With PR][Feature Request] Discard blank attribute
2 participants
@jamesst20 @lessthanjacob