GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-40328: Add tool for generating cjk mapping headers by corona10 · Pull Request #19602 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Apr 19, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue40328

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
bpo-40328: Add tool for generating mapping headers.
a681b40
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 19, 2020
@bedevere-bot bedevere-bot added the awaiting core review label Apr 19, 2020
@corona10 corona10 changed the title bpo-40328: Add tool for generating mapping headers. bpo-40328: Add tool for generating mapping headers Apr 19, 2020
@corona10
corona10 requested a review from vstinner April 19, 2020 13:29
@corona10

corona10 commented Apr 19, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@vstinner
If this repository would not be a proper place to store files under tool/data
Is it okay to create a new repository for this? (it's too huge)
But files need to be stored somewhere in our manageable place since
these files can be broken and loose when the times are passed.
(e.g http://www.info.gov.hk/digital21/eng/hkscs/download/big5-iso.txt is broken now.)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
corona10 requested a review from methane April 19, 2020 13:33
@corona10 corona10 changed the title bpo-40328: Add tool for generating mapping headers bpo-40328: Add tool for generating cjk mapping headers Apr 19, 2020
corona10 added 4 commits April 19, 2020 23:03
@corona10
bpo-40328: Remove
45c4242
@corona10
bpo-40328: nit
b74717e
@corona10
bpo-40328: nit
9a54163
@corona10
bpo-40328: nit
e028c00
@corona10 corona10 added the DO-NOT-MERGE label Apr 20, 2020
corona10 added 3 commits April 20, 2020 17:37
@corona10
bpo-40328: partial refactoring
2e8a7e3
@corona10
bpo-40328: Remove unused class
e357a08
@corona10
bpo-40328: Refactoring genmap_korean.py
ce5414e
vstinner
vstinner reviewed Apr 20, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would prefer to put this script in Tools/unicode/ and don't store Unicode files in Modules/cjkcodecs/tools/data/CP932.TXT, but always download them: as done by Tools/unicode/makeunicodedata.py or some unit tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
corona10 added 6 commits April 20, 2020 23:03
@corona10
bpo-40328: Refactoring
87b146d
@corona10
bpo-40328: Remove unicode files
4e008ee
@corona10
bpo-40328: Update
402c0e6
@corona10
bpo-40328: Update README
c7487f1
@corona10
bpo-40328: Remove unnecessary file
a19a5e9
@corona10
bpo-40328: fix
60e6307
@corona10 corona10 removed the DO-NOT-MERGE label Apr 20, 2020
@corona10
corona10 requested a review from vstinner April 20, 2020 14:48
corona10
corona10 commented Apr 20, 2020
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@vstinner Please take a look :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
corona10 requested a review from serhiy-storchaka April 21, 2020 17:04
@corona10

corona10 commented Apr 21, 2020

Copy link
Copy Markdown
Member Author

@methane @serhiy-storchaka
Also, Can you please take look at this PR?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@methane

methane commented Apr 23, 2020

Copy link
Copy Markdown
Member

I don't have time to understand the script. I only confirmed that this pull request doesn't change the mapping header files.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Apr 23, 2020

Copy link
Copy Markdown
Member Author

@methane
Thank you Naoki San!
The main objective of this PR is generating the same mapping header files :)
For example, we can make some change with this script when
something needs to change about supporting Japanese.
(I don't know the practical case)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Apr 29, 2020

Copy link
Copy Markdown
Member Author

@vstinner Can we merge this PR?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Apr 29, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should we run this script in "make regen-all"? Or do you expect that these generated files will never ever change?

Currently, "make regen-all" has no 3rd party dependencies, whereas this script requires to download files from the Internet. In my experience, any additional external dependency makes Python CIs less stable :-(

Other "unicode scripts" are no run by "make regen-all", so I think that it's fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Apr 29, 2020

Copy link
Copy Markdown
Member

@corona10: I let you decide if you are confident enough to merge this PR with no review.

Since this script is not run by any CI and only run manually, I don't see any risk of regression.

It seems like "Unicode experts" (including me) don't have the bandwidth to review your change.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
corona10 merged commit 113feb3 into python:master Apr 29, 2020
@bedevere-bot bedevere-bot removed the awaiting core review label Apr 29, 2020
@corona10 corona10 mentioned this pull request May 4, 2022
Add tools for generating mappings_XX.h #84508
Closed
@corona10
corona10 deleted the bpo-40328 branch May 11, 2022 05:55
@ezio-melotti ezio-melotti mentioned this pull request May 28, 2022
gh-84508: tool to generate cjk traditional chinese mappings #93272
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@methane methane Awaiting requested review from methane
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@corona10 @methane @vstinner @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.