Skip to content

Cleanup for Beta-version Client Release#224

Merged
joneubank merged 9 commits intodevelopfrom
feat/beta-release-cleanup
Aug 30, 2024
Merged

Cleanup for Beta-version Client Release#224
joneubank merged 9 commits intodevelopfrom
feat/beta-release-cleanup

Conversation

@joneubank
Copy link
Copy Markdown
Contributor

@joneubank joneubank commented Aug 29, 2024

  • Update READMEs for all projects so they are presentable on NPMJS
  • Set all packages to consistent version number, at 2.0.0-beta.1
  • Change exports on client to include a specific list of functionality:
    • All processing functions within the variable process
    • All validation and parsing functions from the validation library, under the variables validate and parse respectively
    • REST Client functionality under the variable rest
    • Common dictionary types and zod schemas from the lectern-dictionary package
    • Common type definitions from lectern-validation library that are used in validation and parse function inputs/outputs

@joneubank joneubank marked this pull request as ready for review August 29, 2024 21:49
Comment thread apps/server/README.md Outdated
Comment thread packages/client/README.md Outdated
Comment thread packages/client/README.md Outdated
Comment thread packages/client/README.md Outdated
/**
* Parsing functions will convert the an object with string values into a new object with all values properly typed
* to match the data types from a schema definition. This parsing process will convert values to numbers, booleans,
* and arrays, as required. String values may also be cleaned up to trim whitespace and match the casing of codeList
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

codeList is a new concept for me. was this term explained anywhere I missed?

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.

Its just an enum of valid values, I am unclear on how this property name was originally selected.

return result;
},
};
const L = loggerFor(__filename);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is a single character variable name really necessary? is that much better or faster or easier to understand/read than logger? of log?

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.

Inherited code, needs review and likely refactor, not touching in this PR. Only appears added because I removed a layer of nesting in this file.

Copy link
Copy Markdown
Member

@justincorrigible justincorrigible left a comment

Choose a reason for hiding this comment

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

aside from the typo comments, the L constant is a no go in my book.
let's discuss it, but I doubt you'll ever convince me single character names are an acceptable idea in the XXI.

Comment thread packages/dictionary/README.md Outdated
Comment thread packages/validation/README.md Outdated
#### Dictionary
Considers all records for all schemas of the dictionary.

- ForeignKey
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this initial capital on purpose? seems to break the snakeCase pattern, so just double checking

Comment thread packages/validation/README.md Outdated
This package provides tools to parse and validate data based on the schemas in Lectern Dictionaries.

## Parsing Data
Parsing data involves reading string values for fields defined in a Lectern Schema and converting the that value into properly typed data. For example, if a field has `"dataType": "number"` and the provided value `"123"` this will be converted from the string value into the numeric `123`. A more complicated example would take a comma separated array value and convert each element and return the final array. If any values cannot be properly parsed and converted based on the schema's rules, an error is returned instead.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Parsing data involves reading string values for fields defined in a Lectern Schema and converting the that value into properly typed data. For example, if a field has `"dataType": "number"` and the provided value `"123"` this will be converted from the string value into the numeric `123`. A more complicated example would take a comma separated array value and convert each element and return the final array. If any values cannot be properly parsed and converted based on the schema's rules, an error is returned instead.
Parsing data involves reading string values for fields defined in a Lectern Schema and converting the values into properly typed data. For example, if a field has `"dataType": "number"` and the provided value `"123"` this will be converted from the string value into the numeric `123`. A more complicated example would take a comma-separated array value, convert each element, and return the final array. If any values cannot be properly parsed and converted based on the schema's rules, an error is returned instead.

@justincorrigible justincorrigible dismissed their stale review August 30, 2024 01:16

postponing the L fixup for a later PR

Comment thread README.md Outdated
| [Lectern Server](apps/server/README.md) | @overture-stack/lectern-server | apps/server/ | [![Lectern GHCR Packages](https://img.shields.io/badge/GHCR-lectern-brightgreen?style=for-the-badge&logo=github)](https://github.com/overture-stack/lectern/pkgs/container/lectern) | Lectern Server web application. |
| [Lectern Client](packages/client/README.md) | @overture-stack/lectern-client | packages/client | [![Lectern Client NPM Package](https://img.shields.io/npm/v/@overture-stack/lectern-client?color=%23cb3837&style=for-the-badge&logo=npm)](https://www.npmjs.com/package/@overture-stack/lectern-client) | TypeScript Client to interact with Lectern Server and Lectern data dictionaries. This library provides a REST client to assist in fetching data from the Lectern server. It also exposes the functionality from the Lectern Validation library to use a Lectern data dictionary to validate data. |
| [Lectern Dictionary](packages/dictionary/README.md) | | @overture-stack/lectern-dictionary | [![Lectern Client NPM Package](https://img.shields.io/npm/v/@overture-stack/lectern-dictionary?color=%23cb3837&style=for-the-badge&logo=npm)](https://www.npmjs.com/package/@overture-stack/lectern-dictionary) | Dictionary meta-schema definition, includes TS types, and Zod schemas. This also exports all utilities for getting the diff of two dictionaries. |
| [Lectern Validation](packages/validation/README.md) | @overture-stack/lectern-validation | packages/validation/ | [![Lectern Validation NPM Package](https://img.shields.io/npm/v/@overture-stack/lectern-client?color=%23cb3837&style=for-the-badge&logo=npm)](https://www.npmjs.com/package/@overture-stack/lectern-client) | Validate data using Lectern Dictionaries. |
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.

Lectern Validation badge URL is pointing to other package, should be something linke @overture-stack/lectern-validation

Comment thread apps/server/README.md Outdated
[<img hspace="5" src="https://img.shields.io/badge/License-AGPL--3.0-blue?style=for-the-badge">](https://github.com/overture-stack/lectern/blob/develop/LICENSE)

Lectern Server is the standalone web service for Lectern. It provides an API to create, manage, and share Data Dictionary schemas.
Lectern Server is a standalone web service that provides an REST API to manage and share Data Dictionary schemas.
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.

Typo on an REST API, should be a

Comment thread README.md
| [Lectern Dictionary](packages/dictionary/README.md) | | @overture-stack/lectern-dictionary | [![Lectern Client NPM Package](https://img.shields.io/npm/v/@overture-stack/lectern-dictionary?color=%23cb3837&style=for-the-badge&logo=npm)](https://www.npmjs.com/package/@overture-stack/lectern-dictionary) | Dictionary meta-schema definition, includes TS types, and Zod schemas. This also exports all utilities for getting the diff of two dictionaries. |
| [Lectern Validation](packages/validation/README.md) | @overture-stack/lectern-validation | packages/validation/ | [![Lectern Validation NPM Package](https://img.shields.io/npm/v/@overture-stack/lectern-client?color=%23cb3837&style=for-the-badge&logo=npm)](https://www.npmjs.com/package/@overture-stack/lectern-client) | Validate data using Lectern Dictionaries. |

## Developer Instructions
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.

On Developer Instructions section the command to build the server using nx should use the full package name, for instance pnpm nx build @overture-stack/lectern-server

Comment thread packages/client/src/index.ts Outdated
* The available processing functions are concerned with data at different scales:
* - processRecord: will process a single data record using schema definition
* - processSchema: will process a collection of data records using a single schema definition
* - processDicitonary: will process multiple collecitons of data records, each vs a different
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.

There is a typo on processDicitonary.

Comment on lines +83 to +85
* Parse and then validate each record in the list. If there are errors found during conversion,
* those errors will be returned and validation will be skipped. The final result will indicate if the
* data processing attempt was successful, or failed due to errors during parsing or validation.
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 description is right, but the params doesn't match what this function is expecting

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.

nice catch... ive started removing the param and return annotations from tsdocs for this reason. let TS manage the types, and just have the comment blocks be descriptive.

Comment on lines +10 to +14
> This may not be the module you are looking to import.
>
> This is a sub-module used as a dependency by both the [Lectern Client](https://www.npmjs.com/package/@overture-stack/lectern-client) and [Lectern Server](https://github.com/overture-stack/lectern/blob/develop/apps/server/README.md).
>
> If you are building an application that will interact with a Lectern Server over HTTP, or wants to validate data using a Lectern Dictionary, you likely want to import the [Lectern Client](https://www.npmjs.com/package/@overture-stack/lectern-client).
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 appreciate this clarification on submodules packages. 💯

There are four separate parsing functions exported, mapping to different collections of data to be processed together:

- `parseFieldValue`: Parse a string value for an individual field.
- `parseRecordValues`: Parse all fields in an [UnprocessedDataRecord](https://github.com/overture-stack/lectern/blob/develop/docs/important-concepts.md#datarecord-and-unprocesseddatarecord) based on a schema definition. Applies `parseFieldValue` to each field.
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 link is pointing to a nonexistent word or WIP glossary.

Copy link
Copy Markdown
Contributor

@leoraba leoraba left a comment

Choose a reason for hiding this comment

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

Well documented I left some comments on some typos. That aside, everything else is excellent.

@joneubank joneubank merged commit 97f1e9b into develop Aug 30, 2024
@joneubank joneubank deleted the feat/beta-release-cleanup branch August 30, 2024 19:38
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

Successfully merging this pull request may close these issues.

3 participants