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

Add support for pre-release and metadata #9

Closed
wants to merge 2 commits into from

Conversation

gyscos
Copy link
Contributor

@gyscos gyscos commented Oct 16, 2018

Also updates dependencies to not crash.

Fixes #8

@wraithan
Copy link
Owner

wraithan commented Jan 19, 2019

The metadata feature is welcome. If I don't hear back from you before next weekend, I'll pull apart the PR and bring in the pre-release/metadata commit and leave the library changes.

Copy link
Owner

@wraithan wraithan left a comment

Choose a reason for hiding this comment

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

I have no issues compiling or running this project and switching the library is unexpected in a PR that just mentions "updates dependencies" with no explanation of what the crash was, why change libraries, or anything. I have to reject this PR as a whole until that is explained or removed from the PR.

@gyscos
Copy link
Contributor Author

gyscos commented Jan 19, 2019

At the time tomllib was crashing when parsing my Cargo.toml. I assumed it was caused by the metadata, but trying again it seems to be something else: it fails to parse nested objects like this:

[dependencies]
foo = "0.1"
bar = "0.2"

[dependencies.baz]
version = "0.3"

Might be joelself/tomllib#1

I mostly picked toml_edit as replacement because that's what cargo-edit uses.

@wraithan
Copy link
Owner

Can you make this PR solely about pre-release and metadata switches and remove anything to do with replacing the library? It is (in my opinion) poor form to include two very different things in a PR. Now that you've provided a problem case I can open an issue and we can see if toml-edit makes the most sense in its own context.

@gyscos
Copy link
Contributor Author

gyscos commented Jan 19, 2019

I added the library as I couldn't test it otherwise.
The PR is already split in 2 independant commits, you're welcome to cherry-pick one half if you prefer.

@wraithan
Copy link
Owner

wraithan commented Jan 21, 2019

I'll be using your metadata/pre_release code as a starting point. NewVersion has felt uncomfortable as the name of the enum for a bit. So I'll be reworking the pre_release and metadata to be passed along as part of a VersionModifier struct. I appreciate the work that went into this patch, including changing the library out to test.

Sorry I was suspicious about that library change. In the wake of what happened with dominictarr/event-stream#116 where a dependency was changed to attack a specific target and the lack detailed reasoning on the original PR left me unsure. I did switch to toml_edit but after refactoring the code to be testable to compare toml_edit and toml-rs I wasn't able to integrate your patch without mostly rewriting it.

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.

2 participants