-
Notifications
You must be signed in to change notification settings - Fork 7
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
add fields to arrival #14
Conversation
This looks good, thanks for working on it. And looking at the code, I realizeI had done less than I thought with fully parsing quakeml, so is good to implement parsing more fields. Technically upgrading the XXXQuantity fields probably will break things, but I think it is the right thing to do. Can you switch your PR to the dev branch instead of main? Reason is the docs at http://crotwell.github.io/seisplotjs/ are built off of main, and don't want to break anything. Once I make a release, dev will become main. |
Glad to help! I updated to merge my commit into dev instead of main. I'll do that with any future PRs I do. |
Maybe just a style question, but I have coded the rest of the objects in quakeml to have only the essential, required parameters on the constructor, and let any optional values be set directly after the constructor. In other words keep the constructor as simple as possible, and use static functions for any "overloading" needs. My main concern is that a constructor with many optional arguments may be harder to use and lead to mistakes, but I am not sure. That may or may not be the best javascript/typescript philosophy. Do you have opinions? So something like this:
versus this:
|
Oh, that makes some sense. To be honest I'm really not enough of a JS/TS programmer to advise on best practices, but even I was starting to be bothered by how many times I had to type each new field I added. 😄 Stand by while I update to your style! |
3b6d63e
to
a07f72e
Compare
Got rid of all the optional ctor params. I also realized I typed one field as a string that could be a DateTime instead. |
How does this look? If you like the general idea I'll do some other elements too.
I think there are some existing cases that are technically "XXXQuantity" where you only parse the value element — will it cause compatibility problems if I "upgrade" them to the RealQuantity class? (Actually, I'd probably have to make that class a template or generic or whatever TypeScript calls it first, to combine RealQuantity, IntegerQuantity and TimeQuantity into one.)