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

Implement PublishViewContextV2 to keep hash optional #1322

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ishitamundhra
Copy link

Pull Request Guidelines

These are recommendations for pull requests.
They are strictly guidelines to help manage expectations.

PR preparation

Run make pr-prep from the root of the repository to run formatting, linting and tests.

Should this be an issue instead
  • is it a convenience method? (no new functionality, streamlines some use case)
  • exposes a previously private type, const, method, etc.
  • is it application specific (caching, retry logic, rate limiting, etc)
  • is it performance related.
API changes

Since API changes have to be maintained they undergo a more detailed review and are more likely to require changes.

  • no tests, if you're adding to the API include at least a single test of the happy case.
  • If you can accomplish your goal without changing the API, then do so.
  • dependency changes. updates are okay. adding/removing need justification.
Examples of API changes that do not meet guidelines:
  • in library cache for users. caches are use case specific.
  • Convenience methods for Sending Messages, update, post, ephemeral, etc. consider opening an issue instead.

@lorenzoaiello
Copy link
Contributor

@ishitamundhra , thanks for opening the PR!

I appreciate you wanting to make this backwards compatible, but I'd like to try and limit additional functions for this and instead bite the bullet once and transition the function to use an input struct instead so that additional parameters can be added in the future without breaking backwards compatibility. How would you feel about going down that path for this PR?

@lorenzoaiello lorenzoaiello added the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Oct 14, 2024
@ishitamundhra
Copy link
Author

ishitamundhra commented Oct 14, 2024

@ishitamundhra , thanks for opening the PR!

I appreciate you wanting to make this backwards compatible, but I'd like to try and limit additional functions for this and instead bite the bullet once and transition the function to use an input struct instead so that additional parameters can be added in the future without breaking backwards compatibility. How would you feel about going down that path for this PR?

Hey @lorenzoaiello. Makes complete sense. I've made the changes to the current diff—can you take a look and let me know if it's in line with what you had in mind?

@lorenzoaiello lorenzoaiello removed the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Oct 14, 2024
@lorenzoaiello lorenzoaiello self-requested a review October 23, 2024 23:12
@lorenzoaiello
Copy link
Contributor

@ishitamundhra , apologies for any ambiguity in my previous post. Thank you for adding the input struct, I was actually proposing not creating a V2 function but creating the breaking change on the current function to transition to an input struct. Does that make sense?

@ishitamundhra
Copy link
Author

@ishitamundhra , apologies for any ambiguity in my previous post. Thank you for adding the input struct, I was actually proposing not creating a V2 function but creating the breaking change on the current function to transition to an input struct. Does that make sense?

Hey @lorenzoaiello, thanks for the feedbacak. Was hesistant to make the breaking change in the first place. Have updated the diff based on your comment, let me know if this feels good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants