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

style: Translate/drop non English comments #518

Merged
merged 3 commits into from Jul 20, 2023
Merged

Conversation

shunkica
Copy link
Collaborator

I always wondered what this said.

@karfau karfau changed the title Translate chinese comment to english chore: Translate todo comment Jul 20, 2023
Copy link
Member

@karfau karfau left a comment

Choose a reason for hiding this comment

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

I already translated it multiple times, but never wanted to mix that change into the PR I was working on...
Did you try to understand if the code surrounding the comment makes sense? Or rather what the comment actually means?

I will check if that branch is covered by any test and if not try to find a sample that triggers it, to understand if it is actually something that we need TODO something about, or if it van just stand as an explanation of the branch.

@karfau
Copy link
Member

karfau commented Jul 20, 2023

Even though I don't really understand the code in that place, it is covered by tests.
I would prefer to drop the "TODO" part of the comment, unless we know what we need to do here.

While we are at it, can you also translate the other comments that are not in English?

@shunkica
Copy link
Collaborator Author

There is only one other non-English comment I could find and it says "忘记闭合" which translates to "forgot to close". Not very helpful :D

@karfau
Copy link
Member

karfau commented Jul 20, 2023

in that case lets drop that comment

@karfau karfau changed the title chore: Translate todo comment style: Translate/drop non English comments Jul 20, 2023
@karfau karfau merged commit 4186661 into xmldom:master Jul 20, 2023
33 checks passed
@shunkica shunkica deleted the translate branch July 20, 2023 18:41
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.

None yet

2 participants