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

Homepage added to Cargo.toml #157

Closed
wants to merge 7 commits into from
Closed

Homepage added to Cargo.toml #157

wants to merge 7 commits into from

Conversation

hiirrxnn
Copy link
Contributor

@hiirrxnn hiirrxnn commented Jan 8, 2024

Let me know if this works !

@Fokko
Copy link
Contributor

Fokko commented Jan 8, 2024

@hiirrxnn I think we want to add the website to each of the Cargo.toml in each of the crates, for example here:

categories = ["database"]
description = "Apache Iceberg Hive Metastore Catalog Support"
repository = "https://github.com/apache/iceberg-rust"
license = "Apache-2.0"
keywords = ["iceberg", "hive", "catalog"]

@hiirrxnn
Copy link
Contributor Author

hiirrxnn commented Jan 9, 2024

Is it alright now ? I've done it in the way mentioned by the doc referenced here . Let me know if something has to be changed here .

@liurenjie1024
Copy link
Contributor

Hi, @hiirrxnn There is another approach for this, please follow these steps:

  1. You can add the home page info in Cargo.toml of workspace.
  2. Then in Cargo.toml of each package, set the filed value to homepage = {workspace = true}.

Here is a great example from cargo: https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table

This way we don't need to repeat same thing for each package.

@hiirrxnn
Copy link
Contributor Author

hiirrxnn commented Jan 9, 2024

I have done that ! Is that what you meant ? Let me know if there's something wrong !

@liurenjie1024
Copy link
Contributor

I have done that ! Is that what you meant ? Let me know if there's something wrong !

Yeah, exactly, thanks!

There are also several fields which are shared among all packages in a workspace:

  1. version
  2. edition
  3. license
  4. repository

Do you want to modify them together or we can do it in later prs, it's up to you.

@hiirrxnn
Copy link
Contributor Author

hiirrxnn commented Jan 9, 2024

I will start doing those other packages asap , but i think i should do that in a different pr as too much modified code in one pr might not be ideal . Please tell me how you think about this , otherwise i can do it in this pr as well !

@hiirrxnn
Copy link
Contributor Author

hiirrxnn commented Jan 9, 2024

Should i follow the exact same approach for all of them , in essence referring them from workspace Cargo.toml?

@liurenjie1024
Copy link
Contributor

Should i follow the exact same approach for all of them , in essence referring them from workspace Cargo.toml?

Yes, thanks!

@liurenjie1024
Copy link
Contributor

I will start doing those other packages asap , but i think i should do that in a different pr as too much modified code in one pr might not be ideal . Please tell me how you think about this , otherwise i can do it in this pr as well !

Cool, looking forward to your next pr!

Copy link
Contributor

@liurenjie1024 liurenjie1024 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!

@hiirrxnn
Copy link
Contributor Author

hiirrxnn commented Jan 9, 2024

Since this PR hadn't been merged yet , the new changes in new PR contains both original issue changes and new ones . Please check that out and let me know if something has to be changed !

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -19,6 +19,9 @@
resolver = "2"
members = ["crates/catalog/*", "crates/iceberg", "crates/test_utils"]

[workspace.package]
homepage = {url = "https://rust.iceberg.apache.org/"}
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that the CI is failing:

Caused by:
  TOML parse error at line 23, column 12
     |
  23 | homepage = {url = "https://rust.iceberg.apache.org/"}
     |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  invalid type: map, expected a string
Suggested change
homepage = {url = "https://rust.iceberg.apache.org/"}
homepage = "https://rust.iceberg.apache.org/"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have fixed this in the new PR , please review those changes and let me know if something has to be improved

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks! It's better to run taplo fmt to format the toml files.

@Fokko
Copy link
Contributor

Fokko commented Jan 9, 2024

I tried to resolve conflicts, but failed 😞

@liurenjie1024
Copy link
Contributor

Seems duplicated with #160. How about we close this pr and focus on #160 only?

@hiirrxnn hiirrxnn closed this Jan 10, 2024
@liurenjie1024
Copy link
Contributor

Thanks! It's better to run taplo fmt to format the toml files.

Good suggestion!

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.

4 participants