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

Renaming PaymentApp* classes #109

Closed
tommythorsen opened this issue Mar 16, 2017 · 10 comments
Closed

Renaming PaymentApp* classes #109

tommythorsen opened this issue Mar 16, 2017 · 10 comments

Comments

@tommythorsen
Copy link
Member

tommythorsen commented Mar 16, 2017

Now that we've renamed to our specification to Payment Handler API, I think it would make sense to not name our classes starting with PaymentApp. I don't think it's super elegant to just rename everything to PaymentHandler*, so I'm looking at alternatives. The first classes I wanted to consider was the following:

5.2 PaymentAppManager interface
5.3 PaymentAppOptions interface
5.4 PaymentAppOption dictionary
5.5 PaymentWallets interface
5.6 WalletDetails interface

I think PaymentAppManager can just become PaymentManager, but PaymentAppOptions can not be renamed to PaymentOptions, as that would cause a name conflict with a class in the Payment Request API specification.

When talking about digital payments, one often talk about a digital wallet containing a set of digital payment instruments. So, how about PaymentInstrument? I realize it's a term with very specific meaning, but I think that it actually fits quite well here.

I would also suggest renaming WalletDetails to be consistent with the other names, so that we would end up with the following set of classes:

5.2 PaymentManager interface
5.3 PaymentInstruments interface
5.4 PaymentInstrument dictionary
5.5 PaymentWallets interface
5.6 PaymentWallet dictionary

WDYT?

@rsolomakhin
Copy link
Collaborator

SGTM

@pjbazin
Copy link
Contributor

pjbazin commented Mar 16, 2017 via email

@adrianhopebailie
Copy link
Contributor

I have an action to go through and fix this as soon as @ianbjacobs and @adamroach have finished the edits they wanted to make before the f2f.

@tommythorsen - If you're already working on it let me know.

@tommythorsen
Copy link
Member Author

@adrianhopebailie: I am not working on this.

Are there major changes incoming? I was thinking to go through the respec warnings and sort those out at some point, but I don't want to create a lot of conflicts for people.

@ianbjacobs
Copy link
Contributor

Hi @tommythorsen,

Let's wait for @adamroach's edits and then do a cleanup pass (e.g., early next week).

Ian

@adamroach
Copy link
Contributor

I'm making these changes in my edits as well. It would be worthwhile for someone to come behind me and make sure I didn't miss anything. :)

adamroach added a commit to adamroach/webpayments-payment-apps-api that referenced this issue Mar 18, 2017
and to flesh out Instrument/Wallet APIs.
These incorporate example code from Marcos'
Payment Handler thumbnail document.
ianbjacobs pushed a commit that referenced this issue Mar 18, 2017
* Proposed resolution to #95 and #96

* Fixing a couple of nits

* Conflict merge

* Changes to resolve issues #99, #109, #111
and to flesh out Instrument/Wallet APIs.
These incorporate example code from Marcos'
Payment Handler thumbnail document.
@tommythorsen
Copy link
Member Author

Most of this has been dealt with by #113, but @adamroach: what's the point of the inconsistently named WalletDetails? Is there a good reason for choosing this name?

Can we either rename WalletDetails to PaymentWallet or PaymentInstrument to InstrumentDetails so that the names match?

@adamroach
Copy link
Contributor

@tommythorsen -- Sorry, no real principled reason for having them different. Feel free to adjust them to be consistent.

MXEBot pushed a commit to mirror/chromium that referenced this issue Mar 31, 2017
@ianbjacobs
Copy link
Contributor

Today we resolved [1] to harmonize using the convention of "Payment" as a prefix. Thus, we chose PaymentWallet and I'll update the spec accordingly.

[1] https://www.w3.org/2017/04/04-apps-minutes#item03

@ianbjacobs ianbjacobs added this to the Close before FPWD milestone Apr 4, 2017
@tommythorsen
Copy link
Member Author

Great! Since you have an action on you to make the change, I don't think we need this issue, so I'll close it.

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

No branches or pull requests

6 participants