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 typos #72314

Merged
merged 12 commits into from
Jul 17, 2022
Merged

Fix typos #72314

merged 12 commits into from
Jul 17, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented Jul 16, 2022

Second commit cleans up trailing whitespaces in files from first commit.

@ghost
Copy link

ghost commented Jul 16, 2022

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

Issue Details

Second commit cleans up trailing whitespaces in files from first commit.

Author: am11
Assignees: -
Labels:

area-System.Reflection.Metadata, community-contribution

Milestone: -

@ghost
Copy link

ghost commented Jul 16, 2022

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

Second commit cleans up trailing whitespaces in files from first commit.

Author: am11
Assignees: -
Labels:

area-Meta, community-contribution

Milestone: -

@jkotas
Copy link
Member

jkotas commented Jul 16, 2022

Test failure is #69125

@@ -7483,7 +7483,7 @@
<opcode name="GCHeapDumpObjectReference" message="$(string.MonoProfilerPublisher.GCHeapDumpObjectReferenceOpcodeMessage)" symbol="CLR_MONO_PROFILER_GC_HEAP_DUMP_OBJECT_REFERENCE_OPCODE" value="70" />
<opcode name="MonitorContention" message="$(string.MonoProfilerPublisher.MonitorContentionOpcodeMessage)" symbol="CLR_MONO_PROFILER_MONITOR_CONTENTION_OPCODE" value="71" />
<opcode name="MonitorFailed" message="$(string.MonoProfilerPublisher.MonitorFailedOpcodeMessage)" symbol="CLR_MONO_PROFILER_MONITOR_FAILED_OPCODE" value="72" />
<opcode name="MonitorAquired" message="$(string.MonoProfilerPublisher.MonitorAquiredOpcodeMessage)" symbol="CLR_MONO_PROFILER_MONITOR_AQUIRED_OPCODE" value="73" />
<opcode name="MonitorAcquired" message="$(string.MonoProfilerPublisher.MonitorAcquiredOpcodeMessage)" symbol="CLR_MONO_PROFILER_MONITOR_AQUIRED_OPCODE" value="73" />
Copy link
Member

Choose a reason for hiding this comment

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

@brianrob Is it ok to fix typos in opcode names here or would that be a breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

@am11 Could you please revert the change in the opcode names for now?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe open an issue on it so that it is not forgotten.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted and opened #72324.

Copy link
Member

Choose a reason for hiding this comment

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

Changing the opcode name could be considered a breaking change, depending upon how the events are consumed. I would not expect the string id to be a breaking change unless someone was looking for a specific string that they've hardcoded somewhere, but I wouldn't worry about that. For the most part, we have not fixed typos for opcodes, keywords, event names, and fields to avoid these types of breaks since there are lots of consumers out there that use string values instead of integer ids.

@am11
Copy link
Member Author

am11 commented Jul 16, 2022

For reference, this time I used a tool called typos in Ubuntu container, to collect the data then used jq to find most recurring typos. This is the self-contained unix script (assumes C compiler / build-essential package on Ubuntu, jq and curl are preinstalled and PWD is root of runtime):

categorize-typos.sh

#!/bin/sh

if [ "$1" = '-h' ] || [ "$1" = "--help" ] || [ "$1" = "-?" ]; then
  echo "Usage: $0 or $0 <to> <from>\n\nExample - to get most recurring typos in a git repo in 20 to 50 window:\n  $0 20 50\n\nDefaults are from: 0 to: 100"
  exit 0
fi

if ! command -v rustc 2>/dev/null; then
  echo "Installing rust .."
  curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
fi

if ! command -v cargo 2>/dev/null; then
  echo "Setting up cargo environment .."
  . "$HOME/.cargo/env"
fi

if ! command -v typos 2>/dev/null; then
  echo "Installing 'typos' crate .."
  cargo install typos-cli
fi

echo "Collecting typos data in list.json .."
# (assuming working directory is root of 'runtime' so it respects .gitignore rules)
typos --format=json > list.json

# we don't use other features of typos tool (such as "apply corrections") -- only collecting data.

echo "Analyzing data using jq .."

# group typos by name, sort by their frequency and display N items (default 0 to 100)
from="${1:-0}" to="${2:-100}"

jq --slurp --argjson from $from --argjson to $to --compact-output \
  '[ group_by(.typo)[] | {word: .[0].typo, count: length, correction: (if .[0].corrections then .[0].corrections | join(", ") else "" end) } ] | sort_by(.count) | reverse | .[$from:$to]' list.json |\
  jq -r '["Count","Word", "Available Corrections"], ["--","------","------"], (.[] | [.count,.word,.correction]) | @tsv'

(I'm sure the last part can be improved and squashed into one jq command, still warming up to jq language syntax 😁)

There were some false-positives so i created a quick (non-exhaustive) .typos.toml file (which typos-cli uses) at root of runtime with these contents (more words can be added):

[default.extend-words]
ba = "ba"
fo = "fo"
nd = "nd"
ot = "ot"
ue = "ue"
ser = "ser"
BA = "BA"
Ba = "Ba"
inout = "inout"
SEH = "SEH"

These two files are for "finding" typos and not checked in, as there is manual work involved for the next step; "fixing" typos.

To fix the correction repo-wide, I used manual find & replace approach: git grep -l $incorrect | xargs -I{} sed -i "s/$incorrect/$correct/g" {}, then reverted changes in restricted places such as src/libraries/*/ref/*, src/native/external, eng/common, *.xlfetc. Sometimes it required renaming the files with typo as well, to find those files, I used: git ls-files | grep -i $incorrect, then moved them manually (using mv or git mv).

@@ -373,7 +373,7 @@ Copyright (c) .NET Foundation. All rights reserved.
Prepare build for ReadyToRun compilations. Builds list of assemblies to compile, and computes paths to ReadyToRun compiler bits
============================================================
-->
<UsingTask Condition="'$(Crossgen2TasksOverriden)' != 'true'" TaskName="PrepareForReadyToRunCompilation" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)"/>
Copy link
Member

Choose a reason for hiding this comment

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

Crossgen2TasksOverriden is used in dotnet/sdk. Fixing this one would have to be coordinated between repos and runtime versions, probably not worth it. Could you please revert this one?

@@ -50,16 +50,16 @@ Tudo pronto? Então, vamos nessa!</String>
<String Id="FailureRestartText">Você deve reiniciar seu computador para concluir a reversão do software.</String>
<String Id="FailureRestartButton">&amp;Reiniciar</String>
<String Id="FailureCloseButton">&amp;Fechar</String>
<String Id="FailureNotSupportedCurrentOperatingSystem">Não há suporte para o [PRODUCT_NAME] neste sistema operacional. Para obter mais informações, confira [LINK_PREREQ_PAGE].</String>
<String Id="FailureNotSupportedX86OperatingSystem">O [PRODUCT_NAME] não tem suporte em sistemas operacionais x86. Instale usando o instalador x86 correspondente.</String>
<String Id="FailureNotSupportedCurrentOperatingSystem">Não há supporte para o [PRODUCT_NAME] neste sistema operacional. Para obter mais informações, confira [LINK_PREREQ_PAGE].</String>
Copy link
Member

Choose a reason for hiding this comment

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

This file is Brazil Portuguese. Is supporte correct? Bing translator shows it with single p.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't noticed it, thanks. It is reverted. I will treat wxl like xlf in the future. :)

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jkotas jkotas merged commit 3ea30ed into dotnet:main Jul 17, 2022
@am11 am11 mentioned this pull request Jul 17, 2022
@am11 am11 mentioned this pull request Jul 31, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants