Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Create ShopperInsights module and move files there. #853
Create ShopperInsights module and move files there. #853
Changes from 4 commits
dd18ea4
5344205
e260bf7
2ab58c4
327293f
3d13dab
cab1d99
7e515b1
3a3df47
c68f92c
91bd96f
44f6ae2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't previously had module-specific READMEs - I'm open to it if we think it's useful and are going to do the same on iOS. What do other think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong preference on where to put it, but I'd like to see a short description of each of the modules and what capabilities they provide to the merchant/user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we want to move to module specific READMEs this should be done in a separate PR where it's added for all modules vs just one. Though I do feel like a README within a module would be generally difficult to find for merchants. We also typically rely on our public docs to communicate capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind showing me an example?
While there's the overall capabilities of the SDK, there's probably a distinction we want to make where we let the merchants know what exactly they get by including a particular module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can totally see a root level MODULES.md or something similar where we can briefly describe what the module does and also what one needs to do to create a new module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example of the PayPal public docs: https://developer.paypal.com/braintree/docs/guides/paypal/overview/android/v4. The overview handles the product overview, then we have the client side + feature specific docs for the client side SDKs as well (our team is responsible for writing these). Typically most features also have a server side integration guide (our team doesn't typically write these) as well as a testing/going live section.
I think we tried having a form of docs live in both the repo and publicly on PPCP but it often resulted in one of the two being out of date so never truly having a "source of truth" for our merchants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option might be to have links on the root readme file to the different module's readme files. This would help with the visibility issue.
If the content on each of the module's readme file is small, it might make more sense in the root readme. This way a developer doesn't need to navigate to see what each module does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just an overview of the module. I don't see it getting out of date. But that's just me.🤷♂️
I can remove the
.md
if that's how we want to roll.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already do that. I probably should have called it out to make it more clear, but yes, that's the intention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to leverage the public dev docs where possible and avoid additional doc maintenance on our team. We're too small and don't have the bandwidth.
If it's common or accepted practice to the Android dev community that there's a README for each individual module, we can include that file with a basic template that points folks to the public dev docs.
Otherwise, let's not adopt this for the SDK.