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.

Sign up for GitHub

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

Jump to bottom

Remove uses of unicode in Rust files #12687

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Remove uses of unicode in Rust files #12687

wants to merge 6 commits into from
+3,489 −3,482

Conversation

brmataptos
Copy link
Contributor

@brmataptos brmataptos commented Mar 26, 2024 •
edited

Description

Fix #12671 by making all Rust source files plain ASCII. This will make some editors happier.

This is staged in 5 commits to facilitate review:

  • [dev impact] update recorded license file used by commit hooks
  • [dev impact] convert files that programmatically add copyright notices to new files
  • [no impact] files requiring only trivial removal of unicode from copyright notices
  • [no impact] files requiring only trivial removal of unicode from copyright notices, that may have lint issues
  • [possible functional impact] files requiring nontrivial changes to remove unicode

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

CI, thorough review, use of scripts to double- and triple-check that most changes only affect copyright messages.

Key Areas to Review

The final commit is nontrivial changes. Most are comments only (including
ASCII art, which I've gone to some pains to preserve), but:

Changes to some files may affect program outputs:

  • aptos-move/aptos-gas-calibration/src/solve.rs
  • network/benchmark/src/lib.rs

A few affect technical documentation and conceivably might be less intelligible:

  • third_party/move/move-examples/diem-framework/crates/crypto/src/unit_tests/ed25519_test.rs
  • third_party/move/move-ir-compiler/move-ir-to-bytecode/syntax/src/lib.rs
  • storage/jellyfish-merkle/src/node_type/mod.rs

These have changes that might affect tests

  • third_party/move/tools/move-unit-test/tests/move_unit_test_testsuite.rs
  • third_party/move/tools/move-unit-test/src/test_reporter.rs
  • third_party/move/move-core/types/src/unit_tests/value_test.rs
  • types/src/network_address/mod.rs

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

@trunk-io Trunk.io
Copy link

trunk-io bot commented Mar 26, 2024 •
edited

⏱️ 3h 40m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-unit-tests 1h 3m 🟥 🟥 🟥 
windows-build 42m 🟩 🟩 
rust-move-unit-coverage 42m 🟥 🟩 🟩 
rust-move-tests 28m 🟩 🟩 🟩 
run-tests-main-branch 13m 🟩 🟩 🟩 
rust-lints 12m 🟥 🟥 🟥 
general-lints 7m 🟩 🟩 🟩 
check-dynamic-deps 6m 🟩 🟩 🟩 
check 5m 🟩 🟥 
semgrep/ci 2m 🟩 🟩 🟩 
file_change_determinator 34s 🟩 🟩 🟩 
file_change_determinator 31s 🟩 🟩 🟩 
file_change_determinator 12s 🟩 
permission-check 11s 🟩 🟩 🟩 
permission-check 10s 🟩 🟩 🟩 
permission-check 8s 🟩 🟩 🟩 
permission-check 7s 🟩 🟩 🟩 

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
rust-move-tests 3s 5m -99%

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

@brmataptos brmataptos changed the title Rmove uses of unicode in Rust files Remove uses of unicode in Rust files Mar 26, 2024
@brmataptos brmataptos marked this pull request as ready for review March 26, 2024 19:21
@brianolson
Copy link
Contributor

"some editors"
Which editors?
Supporting editors that don't work with utf8 feels like we're still writing webapps for IE6

@codecov Codecov
Copy link

codecov bot commented Mar 26, 2024 •
edited

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 62.6%. Comparing base (3eb19c4) to head (d11623f).

Files Patch % Lines
aptos-move/aptos-sdk-builder/src/rust.rs 0.0% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##             main   #12687       +/-   ##
===========================================
- Coverage    69.2%    62.6%     -6.6%     
===========================================
  Files        2293      819     -1474     
  Lines      436530   183551   -252979     
===========================================
- Hits       302391   115053   -187338     
+ Misses     134139    68498    -65641

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brmataptos brmataptos mentioned this pull request Mar 26, 2024
Merged
21 tasks
@JoshLind
Copy link
Contributor

LGTM @brmataptos 😄 Note: you'll need to rebase (#12690 was just landed) and update the failing tests/jobs above. Once you've done that, I can approve and we should be good to land.

@brianolson
Copy link
Contributor

"Rust input is interpreted as a sequence of Unicode code points encoded in UTF-8."
https://doc.rust-lang.org/reference/input-format.html
We have no need to support editors that can't deal with valid Rust source.

@brmataptos
Copy link
Contributor Author

"Rust input is interpreted as a sequence of Unicode code points encoded in UTF-8." https://doc.rust-lang.org/reference/input-format.html We have no need to support editors that can't deal with valid Rust source.

Buy me a Linux machine to work on and I'll pay attention to your noise. Forced to use a Mac, we all have to make concessions to the inadequate tooling.

@brianolson
Copy link
Contributor

image
?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alinush alinush Awaiting requested review from alinush alinush is a code owner

@msmouse msmouse Awaiting requested review from msmouse msmouse is a code owner

@JoshLind JoshLind Awaiting requested review from JoshLind JoshLind is a code owner

@vgao1996 vgao1996 Awaiting requested review from vgao1996 vgao1996 is a code owner

@gregnazario gregnazario Awaiting requested review from gregnazario gregnazario is a code owner

@brianolson brianolson Awaiting requested review from brianolson brianolson is a code owner

@geekflyer geekflyer Awaiting requested review from geekflyer geekflyer is a code owner

@gelash gelash Awaiting requested review from gelash gelash is a code owner

@zjma zjma Awaiting requested review from zjma zjma is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Avoid using unicode in aptos-core, to reduce chances of bugs in IDEs
3 participants
@brmataptos @brianolson @JoshLind