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

feat(rust): Update Cargo.lock, preserve formatting in Cargo.{toml,lock} #705

Merged
merged 15 commits into from
Jan 15, 2021

Conversation

fasterthanlime
Copy link
Contributor

The initial Rust support worked, but it didn't preserve formatting in Cargo.toml, because it parsed the whole file using @iarna/toml and serialized it again, turning inputs like these:

[dependencies]
# revert to upstream once https://github.com/seanmonstar/warp/pull/753 lands	
warp = { version = "0.2.5", registry = "netlify-trove" }

Into this:

  [dependencies.warp]
  version = "0.2.5"
  registry = "netlify-trove"

(note: the spacing is part of the output).

The problem is not so much that it reformats the whole file (also that's annoying too), but that an important comment was lost in translation.

Additionally, with the initial support, Cargo.lock was not updated. This resulted in follow-up commits (whenever developers ran cargo check, cargo build, cargo test etc. locally) that had "Cargo.lock update noise", but also, it prevented a cargo publish step from ever working on CI for non-monorepos that were binary crates (and thus have their Cargo.lock file committed).

This PR builds on @iarna/toml's low-level API: it introduces a TaggedTOMLParser, which returns objects instead of string values. Those objects contain the location of the string in the original .toml file. A single function, replaceTomlString, is exported, which parses a TOML payload, attempts to follow a path into the "object tree", replaces the string, and validates that the result is valid TOML.

Both the CargoToml and CargoLock updaters now use that function. Additionally, the code was reorganized a little, some comments were added, common parts were extracted to their own files.

The rust releaser changes are fairly simple, and its existing tests are mostly untouched (modulo updating the snapshots, since it now preserves formatting!), I'm going to add some more tests to make sure it works both with Cargo.lock present and absent, both in a monorepo and non-monorepo scenario.

Fixes #704 🦕

@fasterthanlime fasterthanlime requested a review from a team as a code owner January 14, 2021 18:49
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 14, 2021
@fasterthanlime
Copy link
Contributor Author

Another note: this also includes some partial typings for @iarna/toml's low-level interface, which is not covered by the @types/iarna__toml package unfortunately.

Since this is the first "typing extensions" for this repo, I had to touch tsconfig.json, here's the relevant docs: https://www.typescriptlang.org/tsconfig#typeRoots

@codecov
Copy link

codecov bot commented Jan 14, 2021

Codecov Report

Merging #705 (fdb7745) into master (5a83db1) will increase coverage by 0.91%.
The diff coverage is 98.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
+ Coverage   85.86%   86.77%   +0.91%     
==========================================
  Files          52       55       +3     
  Lines        6204     6579     +375     
  Branches      576      616      +40     
==========================================
+ Hits         5327     5709     +382     
+ Misses        876      869       -7     
  Partials        1        1              
Impacted Files Coverage Δ
src/updaters/rust/toml-edit.ts 96.34% <96.34%> (ø)
src/updaters/rust/cargo-toml.ts 99.28% <99.28%> (ø)
src/releasers/rust.ts 94.11% <100.00%> (+0.59%) ⬆️
src/updaters/rust/cargo-lock.ts 100.00% <100.00%> (ø)
src/updaters/rust/common.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a83db1...fdb7745. Read the comment docs.

@bcoe
Copy link
Contributor

bcoe commented Jan 14, 2021

Another note: this also includes some partial typings for @iarna/toml's low-level interface, which is not covered by the @types/iarna__toml package unfortunately.

@fasterthanlime seems reasonable as a stop gap, but could we open a tracking ticket on DefinitelyTyped?


Let me know when you've added all the tests you'd like and I'll review.

@fasterthanlime
Copy link
Contributor Author

@fasterthanlime seems reasonable as a stop gap, but could we open a tracking ticket on DefinitelyTyped?

DefinitelyTyped/DefinitelyTyped#50618

@fasterthanlime
Copy link
Contributor Author

codecov is happy, I'm mostly happy - still going to add some unit tests to toml-edit, but I think you're good to start a proper review @bcoe 🎉

@fasterthanlime
Copy link
Contributor Author

So, the windows tests are failing in CI, I got curious and ran them locally (Windows 10, node v15.5.0) and they run just fine here. It's a mystery! 🔎

@bcoe
Copy link
Contributor

bcoe commented Jan 15, 2021

@fasterthanlime I think you're probably getting bitten by line endings, I've had to add replace('\r\n', '\n'); in a few places when taking snapshots.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

This is looking good to me, and I'm comfortable landing the way it is.

The only real concern I have is if we bump into any significant bugs with the TOML parser, and need to keep fleshing out that test suite -- it might eventually make sense for it to be its own repo.

@@ -4,8 +4,10 @@
"lib": ["es2018", "dom"],
"rootDir": ".",
"outDir": "build",
"typeRoots": ["./typings", "./node_modules/@types"]
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL, I've never gotten this fancy with providing custom typings.

@bcoe
Copy link
Contributor

bcoe commented Jan 15, 2021

@fasterthanlime went ahead and addressed the newline issues for you, IMO this is ready to go. Shall I get it released for you tomorrow?

@fasterthanlime
Copy link
Contributor Author

@bcoe sounds good to me! Thanks for addressing the newline issue, I keep forgetting Git can be configured to check out with CRLF 🙈

I share your general concern re "parsers vs real world input", although 1) TOML is a relatively simple format compared to, say, YAML 2) I'm happy I found a solution based on that same parser, and not something entirely custom or a pile of regexps.

@fasterthanlime
Copy link
Contributor Author

Just thought of some more good test cases for the toml-edit part (mostly re: quoted keys and non-basic strings), maybe hold off on the release until I add them in ⏳

Supports multi-line strings, literal strings (single-quoted), string
escapes, etc.
Simpler mechanism to hook into the parser.
More toml-edit tests.
@fasterthanlime
Copy link
Contributor Author

Glad I took the time to look at some of the funkier TOML out there, all kinds of strings are now supported, I just want to add support for platform-specific dependencies before this lands: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies

@fasterthanlime
Copy link
Contributor Author

Platform-specific dependency support is in, adding a few more tests for good measure... ⌛

@fasterthanlime
Copy link
Contributor Author

All unit tests are here, running this against a sample repo to be 101% sure.

@fasterthanlime
Copy link
Contributor Author

Ran successfully on a couple repositories - a complex private one, and this simple public one: fasterthanlime/release-please-sample#1

@bcoe This PR is ready for prime time! 🚀🐿

@bcoe bcoe merged commit 198c327 into googleapis:master Jan 15, 2021
@bcoe
Copy link
Contributor

bcoe commented Jan 15, 2021

@fasterthanlime thanks for the contribution 👍 I might just need to learn rust.

@fasterthanlime
Copy link
Contributor Author

@bcoe thanks for maintaining this repo! Contributing has been a real pleasure ☺️

If you do find some time to learn some Rust, I have quite a few articles you may enjoy ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rust: update Cargo.lock and attempt formatting-preserving Cargo.toml update
2 participants