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 kana producing bug #44

Merged
merged 6 commits into from
Nov 24, 2021
Merged

Fix kana producing bug #44

merged 6 commits into from
Nov 24, 2021

Conversation

onting
Copy link
Collaborator

@onting onting commented Nov 23, 2021

This patch should close issue #42 by resolving '1' and '2' (others meight be handled in engine).
It changes single file: hanjpautomata.c
The patch,

  1. refines comments.
  2. fixes am_base_to_kana.

It changes automata comment to more readable.
It fixes am_push by fixng row index selecting code.
It fixes am_push by adding missing conditions into Choseong/Jungseong eatting step.
@JSYoo5B
Copy link
Member

JSYoo5B commented Nov 23, 2021

When you're ready to merge this, I'll apply this bug fix to my PR(#43).

@onting
Copy link
Collaborator Author

onting commented Nov 23, 2021

Test is failing. I'm fixing.

@onting onting marked this pull request as draft November 23, 2021 23:49
It fixes junseong division section
@onting onting marked this pull request as ready for review November 24, 2021 00:05
@onting onting requested a review from JSYoo5B November 24, 2021 00:09
@onting
Copy link
Collaborator Author

onting commented Nov 24, 2021

I consider merging jungseong divide step and jungseong eatting step into one step would make code more natrual.
However, it makes this step more complecated(so I didn't do it).
How do you think about it? @JSYoo5B

@JSYoo5B
Copy link
Member

JSYoo5B commented Nov 24, 2021

I consider merging jungseong divide step and jungseong eatting step into one step would make code more natrual. However, it makes this step more complecated(so I didn't do it).

Can you notate the corresponding code block about this?

@JSYoo5B
Copy link
Member

JSYoo5B commented Nov 24, 2021

FYI, my plans for refactorings are following:

  1. rename some local variables. (just as what you saw in currently uploaded commits)
  2. extract some internal steps into internal functions (for better readability)
  3. maybe some extra jobs when it seems necessary

As you see, your question may be important for my second job.

@onting
Copy link
Collaborator Author

onting commented Nov 24, 2021

The step would be like below

// Eat Jungseong
ch = buffer->jung; // victim
buffer->jung = 0;
switch(ch) {
    case HANJP_JUNGSEONG_WA:
       ...some logical decision
    case HANJP_JUNGSEONG_YA:
    case HANJP_JUNGSEONG_A:
    j = HANJP_VOWEL_A; break;
    ....
    default:
    return FALSE;
}

The code means the step would include logical decision for w, y implying Jungseong and row index modifying staments.
I felt it is ugly.

@onting
Copy link
Collaborator Author

onting commented Nov 24, 2021

For example, ㅘ, ㅑ, ㅛ, ㅠ need logical decision and row index modification at column index selection.

@onting
Copy link
Collaborator Author

onting commented Nov 24, 2021

Sorry, there was remaining faults.
It meight be clean now.

@JSYoo5B
Copy link
Member

JSYoo5B commented Nov 24, 2021

Well, I understood as:

  1. Jungseong dividing step, which needs to consider double jungseong.
    As a result, nothing may happen or split double jungseong into separate character, or modifying consonant indexing (especially for Y-family)
  2. Eating step, which consumes character (in this context, we're considering jungseong, the vowels)
    As a result, it calculates the vowel index in Kana.

Yes. They're relevant. But I think splitting would be better.

  1. We still have some ugly code blocks which have if statements inside switch-case.
  2. As we discussed above, they have a separate purpose. (although they're strongly coupled.)
  3. This is not a real-time or performance-critical job. Readability is more important.

@onting
Copy link
Collaborator Author

onting commented Nov 24, 2021

You understood it proferly.
Okay, thank you for help. I agree with you.

Copy link
Member

@JSYoo5B JSYoo5B left a comment

Choose a reason for hiding this comment

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

Seems fine.

But next time, maybe splitting PRs into its purpose would be better.
This PR tries to fix bugs.
I'd recommend modifying only necessary comments. (the comments which are outdated, or provide wrong information about code)
When you think the comment modification is necessary for the PR, (even though the purpose of PR isn't documentation nor refactoring) It'd be better to split commit.

@onting
Copy link
Collaborator Author

onting commented Nov 24, 2021

Okay, good pointing out. I will remind it later.

@onting onting merged commit 6a93132 into develop Nov 24, 2021
@onting onting deleted the fix-kana-producing branch November 24, 2021 01: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.

2 participants