Skip to content
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

fix: tag parsing for reopening #26

Merged
merged 1 commit into from
Oct 28, 2024
Merged

fix: tag parsing for reopening #26

merged 1 commit into from
Oct 28, 2024

Conversation

nabalone
Copy link
Collaborator

@nabalone nabalone commented Oct 28, 2024

This change is Reviewable

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 21 of 21 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nabalone)


components/language-chooser/common/find-language/languageTagUtils.spec.ts line 37 at r1 (raw file):

describe("get shortest equivalent version of langtag", () => {
  it("should return the shortest tag if it exists", () => {

What should happen if it doesn't exist?
Same for maximal.


components/language-chooser/common/find-language/README.md line 129 at r1 (raw file):

### Language data processing pipeline

If you modify [langtagProcessing.ts](./langtagProcessing.ts), run `npm run find-language/common/langtag-processing` to update [languageData.json](language-data/languageData.json) and [equivalentTags.json](language-data/equivalentTags.json).

Should we be deleting shortestTagLookups.json?


components/language-chooser/react/language-chooser-react-mui/src/demos/DialogDemo.tsx line 115 at r1 (raw file):

    props.alreadyFilled ? samplePrefilledSelections : ({} as IOrthography)
  );
  // Language tag can also be computed from the IOrthography object using createTagFromOrthography

Looks like you decided to use the function here now?
So I think this comment is stating you could do what you are doing?
So it ends up confusing.

Copy link
Collaborator Author

@nabalone nabalone left a comment

Choose a reason for hiding this comment

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

Reviewable status: 16 of 22 files reviewed, 3 unresolved discussions (waiting on @andrew-polk)


components/language-chooser/common/find-language/languageTagUtils.spec.ts line 37 at r1 (raw file):

Previously, andrew-polk wrote…

What should happen if it doesn't exist?
Same for maximal.

Done. Returns undefined


components/language-chooser/common/find-language/README.md line 129 at r1 (raw file):

Previously, andrew-polk wrote…

Should we be deleting shortestTagLookups.json?

Done.


components/language-chooser/react/language-chooser-react-mui/src/demos/DialogDemo.tsx line 115 at r1 (raw file):

Previously, andrew-polk wrote…

Looks like you decided to use the function here now?
So I think this comment is stating you could do what you are doing?
So it ends up confusing.

Done. Yeah, oops

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nabalone)

@andrew-polk andrew-polk merged commit 3ebd00a into main Oct 28, 2024
1 check passed
@andrew-polk andrew-polk deleted the langtag_parsing branch October 28, 2024 22:01
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.

2 participants