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

Issue #5826: Create observability-primer.md (French version) #5833

Conversation

iguitton
Copy link
Contributor

Translated the observability-primer.md into French

@iguitton iguitton requested a review from a team as a code owner December 20, 2024 14:45
@opentelemetrybot opentelemetrybot requested review from a team December 20, 2024 14:45
@iguitton
Copy link
Contributor Author

iguitton commented Dec 20, 2024

Hi @svrnm ,

As mentioned in the PR #5386 (comment), the spell checker is working fine, but it seems that the dictionary lacks many common French words. This is currently blocking the merge.
Adding the 152 missing words to the dictionary for this PR doesn't seem like a sustainable solution. Do you have any suggestions for addressing this issue more effectively?

Thanks,

Isa

@iguitton
Copy link
Contributor Author

iguitton commented Jan 2, 2025

@dmathieu, @codeboten, @marcalff, @theletterf, @cartermp, @austinlparker, @chalin and @svrnm: This PR is ready to be reviewed.

@bertysentry
Copy link
Contributor

Would it be okay if we imported the entire French dictionary into fr-mots.txt to avoid the cumbersome task of adding every single word in the French documentation manually?

@iguitton
Copy link
Contributor Author

iguitton commented Jan 3, 2025

Would it be okay if we imported the entire French dictionary into fr-mots.txt to avoid the cumbersome task of adding every single word in the French documentation manually?

@bertysentry , importing the entire French dictionary could address many issues, but we would still need to add words manually because the spell checker:

  • Is case-sensitive: It treats "Collecteur" and "collecteur" as distinct words.
  • Does not handle plural or gender variations: Singular and plural forms, as well as masculine and feminine versions, need to be added separately (e.g., "autre" and "autres"; "distribué", "distribuée", "distribués", and "distribuées").
  • Does not account for verb conjugations: All conjugated forms of verbs must be explicitly added to pass the spell checker test.
  • Does not recognize words with prefixes like "d'" or "l'": These are treated as new words, so both "autre" and "d'autre" need to be added to fr-mots.txt.

While importing a dictionary would reduce the workload, these limitations would still require manual intervention.

@chalin
Copy link
Contributor

chalin commented Jan 4, 2025

Adding the 152 missing words to the dictionary for this PR doesn't seem like a sustainable solution.

I agree that it isn't optimal nor sustainable.

Do you have any suggestions for addressing this issue more effectively?

Submitting PR(s) upstream to https://github.com/streetsidesoftware/cspell-dicts; for example, the last update seems to be streetsidesoftware/cspell-dicts#680. Any volunteers? It would be of benefit to all projects that use cSpell's fr_FR dictionaries.

I'm going to create a separate issue to track this... --> #5870

content/fr/concepts/observability-primer.md Outdated Show resolved Hide resolved
@opentelemetrybot opentelemetrybot requested a review from a team January 6, 2025 08:58
@chalin chalin force-pushed the feature/issue-5826-create-observability-primer.md branch from 01cdd15 to d079c74 Compare January 6, 2025 17:08
@chalin
Copy link
Contributor

chalin commented Jan 6, 2025

/fix:format

@opentelemetrybot
Copy link
Collaborator

You triggered fix:format action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/12636968824

@opentelemetrybot
Copy link
Collaborator

fix:format was successful.

IMPORTANT: (RE-)RUN /fix:all to ensure that there are no remaining check issues.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

@iguitton - could you please address the issue highlighted by @bertysentry in #5870 (comment), strip the fr-mots.txt to a minimum, and see if it all works?

@bertysentry
Copy link
Contributor

@iguitton You know where to reach me if needed 😉

- Added fr-fr in .cspell.yaml
- Skimmed the fr-mots.txt file to retain only the observability-specific terminology.
@opentelemetrybot opentelemetrybot requested a review from a team January 7, 2025 08:31
@opentelemetrybot opentelemetrybot requested a review from a team January 7, 2025 08:31
@iguitton
Copy link
Contributor Author

iguitton commented Jan 7, 2025

@iguitton - could you please address the issue highlighted by @bertysentry in #5870 (comment), strip the fr-mots.txt to a minimum, and see if it all works?

@chalin : As suggested by @bertysentry , I added fr-fr in the .cspell.yaml file and skimmed the fr-mots.txt file to retain only the observability-specific terminology. Only a few unknown words are now reported by the spelling check which is the expected behavior.

Thank you both for your help.

Copy link
Contributor

@bertysentry bertysentry left a comment

Choose a reason for hiding this comment

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

Awesome! 😊

Took marcalff's comment into account
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Great work,

I finally understood what we are working on ;-)

@opentelemetrybot opentelemetrybot requested a review from a team January 7, 2025 14:10
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

I'm very glad that we got the dictionary issues sorted out. Thanks again @bertysentry for spotting that!

@chalin chalin merged commit eeb59f5 into open-telemetry:main Jan 7, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants