Skip to content

0.1.0#14

Merged
NivGreenstein merged 274 commits intomasterfrom
0.1.0
Sep 30, 2024
Merged

0.1.0#14
NivGreenstein merged 274 commits intomasterfrom
0.1.0

Conversation

@NivGreenstein
Copy link
Copy Markdown
Contributor

@NivGreenstein NivGreenstein commented Sep 10, 2024

Question Answer
Bug fix
New feature
Breaking change
Deprecations
Documentation
Tests added
Chore

This is the implementation of the new architecture of Geocoding API. It includes Control grid search and location search.

…to camelCase CommonRequestParameters infered type
Comment on lines +77 to +80
"files": {
"__name": "S3_FILES_DATA",
"__format": "json"
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supposed to be a large file, don't we want to provide it in another way?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the config and metadata for the files that are being stores in S3.
This way the service knows what bucket and file's name to pull.

Comment thread config/default.json
Comment on lines +31 to +34
"node": "http://control_elastic:9200",
"auth": {
"username": "control",
"password": "password"
"username": "elastic",
"password": "changeme"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should stay as default values. These seem to be actual credentials (even if for local work).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elastic and changeme are the default Elasticsearch credentials.
control_elastic is just the placeholder. for local env, you need to change it to localhost if you are running it locally.

Comment thread config/default.json
Comment on lines +42 to +45
"node": "http://geotext_elastic:9200",
"auth": {
"username": "geotext",
"password": "password"
"username": "elastic",
"password": "changeme"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Comment thread config/test.json
Comment on lines +68 to 74
"controlObjectDisplayNamePrefixes": {
"TILE": "Tile",
"SUB_TILE": "Sub Tile",
"ROUTE": "Route",
"ITEM": "Item",
"CONTROL_POINT": "Control Point"
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed? You could do the same as you do with snake to camel and vice versa and seperate by spaces.

Copy link
Copy Markdown
Contributor Author

@NivGreenstein NivGreenstein Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a dictionary to the feature's generated display name.
I'll change the config's name to a better name

"F_CODE": 8754111,
"F_ATT": 951111,
"ENTITY_HEB": "control point",
"LAYER_NAME": "CONTROL_GIL_GDB.CTR_CONTROL_POINT_CROSS_N",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure the namings are ok.

Comment thread devScripts/importDataToElastic.ts
Comment thread devScripts/importDataToElastic.ts Outdated
/* eslint-disable @typescript-eslint/no-unsafe-assignment */
/* eslint-disable @typescript-eslint/naming-convention */
/* eslint-disable @typescript-eslint/no-unsafe-member-access */
import crypto from 'crypto';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this package is deprecated.
Maybe we can use uuid instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dev-scripts. But, will change to node:crypto

Comment thread openapi3.yaml
- x-api-key: []
x-user-id: []

/search/control/tiles:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest splitting endpoints for querying tile and mgrs so we could have clearer and simpler API handling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This route enables search Control Tiles via providing a MGRS Tile. It will result with all of the tiles that intersects with our Control Tiles.

Comment thread src/control/utils.ts

const LAST_ELEMENT_INDEX = -1;

const generateDisplayName = <T extends Tile | Item | Route>(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create an enum for the common used strings representing types and other entities.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like you to elaborate :)

Comment thread src/control/interfaces.ts Outdated
@NivGreenstein NivGreenstein merged commit 8d9694c into master Sep 30, 2024
@NivGreenstein NivGreenstein mentioned this pull request Sep 30, 2024
@NivGreenstein NivGreenstein deleted the 0.1.0 branch October 19, 2024 08:10
@NivGreenstein NivGreenstein restored the 0.1.0 branch October 19, 2024 08:10
@syncush syncush deleted the 0.1.0 branch October 29, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants