Hello, thanks for submitting this tap. The code looks very straightforward (which is great!), and the data seem useful. In reviewing the tap, we came upon some issues, which are summarized here. We'd be looking for these things to be cleaned up before accepting the submission. Thank you!
[ ] The config values that are more static should have reasonable defaults
It looks like the only parameters that are required for the config are the api key and base ID, reducing the amount of choices a user needs to make by providing a reasonable default value is a good UX improvement here (e.g., config.get("metadata_url", "https://api.airtable.com/v2/meta/") so the tap defaults to the current URL but allows an override in the config)
[ ] Discovered Schema includes non-JSON schema keys (e.g., "selected" and "name")
"Selected should be a piece of metadata written outside the tap for each stream and field and read by the sync function to provide field and stream selection
See tap_adroll/discover.py for a boilerplate example and tap_adroll/sync.py for stream selection and field selection usage, respectively, with the get_selected_streams on the Catalog class and the Transformer just below that.
[ ] Discovery Mode Doesn't Create a Valid CatalogEntry
See above, the metadata should be a list of singer metadata entries, and the generated JSON schema should be passed in as the schema parameter
This code assumes a hard coded config path, and should instead only pass the config into the "run_discovery" and "run_sync" methods, so that the config file that was passed in as a CLI argument is used throughout.
Right now, if that config.json file is renamed, the tap will fail.
Not blocking, but related, generally a config sample in the repo is called config.sample.json to make clear that the user needs to modify it.
[ ] --catalog not supported
I see an existing issue for this, but to elaborate a bit, right now the tap only works with the --properties CLI option, which is deprecated and should not be used.
Instead, specifying --catalog and working with the standard Catalog class in the code provides some common functionality using the singer-python functions
[ ] "selected_by_default" config parameter doesn't work
While this is not a Singer standard, and should default to false in favor of metadata-based selections. I understand that it is very helpful for development, and I like the feature in general.
Potential Feature Request
[ ] Not a blocking issue, but maybe a nice-to-have. Is there a way to discover all base_ids available to the tap user and generate a catalog entry for each? This could be helpful since these values are pretty strange to look at, and likely cumbersome to specify one at a time.
Hello, thanks for submitting this tap. The code looks very straightforward (which is great!), and the data seem useful. In reviewing the tap, we came upon some issues, which are summarized here. We'd be looking for these things to be cleaned up before accepting the submission. Thank you!
config.get("metadata_url", "https://api.airtable.com/v2/meta/")so the tap defaults to the current URL but allows an override in the config)get_selected_streamson theCatalogclass and theTransformerjust below that.CatalogEntryschemaparameterconfig.jsonfile is renamed, the tap will fail.config.sample.jsonto make clear that the user needs to modify it.--catalognot supported--propertiesCLI option, which is deprecated and should not be used.--catalogand working with the standardCatalogclass in the code provides some common functionality using thesinger-pythonfunctionsfalsein favor of metadata-based selections. I understand that it is very helpful for development, and I like the feature in general.Potential Feature Request
base_ids available to the tap user and generate a catalog entry for each? This could be helpful since these values are pretty strange to look at, and likely cumbersome to specify one at a time.