Samples for search #11338
Samples for search #11338
Conversation
| @@ -39,21 +39,21 @@ def __init__(self, endpoint, credential, **kwargs): | |||
| endpoint=endpoint, sdk_moniker=SDK_MONIKER, **kwargs | |||
| ) # type: _SearchServiceClient | |||
|
|
|||
| def __enter__(self): | |||
| async def __aenter__(self): | |||
There was a problem hiding this comment.
messed up aio in indexer :(
|
@rakshith91 can you update the samples readme as well: |
sdk/search/azure-search-documents/samples/async_samples/sample_indexers_operations_async.py
Show resolved
Hide resolved
|
@heaths @rakshith91 can we get this merged today to support the UX study, and make any necessary issues for other things to follow-up on? |
There was a problem hiding this comment.
I'm signing off so you can make headway for the UX study, but the class names and related method names aren't correct based on what we decided previously. I would be better if these were correct for the UX study so we can get accurate feedback (e.g. maybe they are indeed too long, which is good feedback for the archboard).
| index_name = "hotels" | ||
| fields = [ | ||
| SimpleField(name="hotelId", type=edm.String, key=True), | ||
| SimpleField(name="baseRate", type=edm.Double) |
There was a problem hiding this comment.
Just for what looks like a more complete (enough) example, maybe add a hotelName that is a SearchableField with a few bells and whistles. As a consumer, this index would give me nothing useful (I'd see a base rate, but for whom?).
| SimpleField(name="baseRate", type=edm.Double) | ||
| ] | ||
| index = Index(name=index_name, fields=fields) | ||
| ind_client = service_client.get_indexes_client() |
There was a problem hiding this comment.
Eventually, the method name should comply with the guidelines, i.e. that the model name is part of not only the client name ({Model}Client), but the method is get_{Model}Client (granted, in snake case idiomatic of python). Might consider doing before the UX study so, if people say "that's too long", we can take that back to archboard.
| index = Index(name=index_name, fields=fields) | ||
| ind_client = service_client.get_indexes_client() | ||
| async with ind_client: | ||
| index = await ind_client.create_index(index) |
There was a problem hiding this comment.
The methods - now on sub-clients - should not repeat the model. I.e. just create. Same as below for other sub-clients.
| data_source = await ds_client.create_datasource(ds) | ||
|
|
||
| # create an indexer | ||
| indexer = Indexer(name="async-sample-indexer", data_source_name="async-indexer-datasource", target_index_name="hotels") |
| # [START create_indexer_async] | ||
| # create a datasource | ||
| ds_client = service_client.get_datasources_client() | ||
| credentials = DataSourceCredentials(connection_string=connection_string) |
There was a problem hiding this comment.
This connection string should just go onto the SearchIndexerDataSource. See #10860.
| # create a datasource | ||
| ds_client = service_client.get_datasources_client() | ||
| credentials = DataSourceCredentials(connection_string=connection_string) | ||
| container = DataContainer(name='searchcontainer') |
There was a problem hiding this comment.
SearchIndexerDataContainer. See #10860.
| ds_client = service_client.get_datasources_client() | ||
| credentials = DataSourceCredentials(connection_string=connection_string) | ||
| container = DataContainer(name='searchcontainer') | ||
| ds = DataSource(name="async-indexer-datasource", type="azureblob", credentials=credentials, container=container) |
There was a problem hiding this comment.
SearchIndexerDataSource. See #10860.
| @@ -0,0 +1,187 @@ | |||
| { | |||
There was a problem hiding this comment.
Did you already have samples using medical data? Seems most others have been using the hotels sample. As long as you're consistent, though. If this is the first of new samples, you might consider a consistent model (i.e. hotels) for similarity across languages' samples.
There was a problem hiding this comment.
Having skimmed the contents as well, I'm a little worried the topic of conversation might be triggering to people who have been impacted by this tragedy. Hotels seem pretty safe.
There was a problem hiding this comment.
changed it to use hotel data - agree with patient data being sensitive - yes i already had the data for a different project i've been working on
| SimpleField(name="estimatedonsetdate", type=edm.String), | ||
| SimpleField(name="gender", type=edm.String, filterable=True), | ||
| SimpleField(name="nationality", type=edm.String), | ||
| SimpleField(name="notes", type=edm.String), |
There was a problem hiding this comment.
Why all SimpleFields? At leaset for this one, using a SearchableField not only provides some diversity but would also set a better recommendation for such fields that people might want to use analyzers.
| cors_options = CorsOptions(allowed_origins=["*"], max_age_in_seconds=60) | ||
|
|
||
| # pass in the name, fields and cors options and create the index | ||
| index = Index( |
There was a problem hiding this comment.
Yes I think it is best to merge this PR, then do the updates in the other PR?
There was a problem hiding this comment.
10860 should be done in a different PR
|
will work on renaming in a different pr |
No description provided.