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

Generate Markdown table of tools #473

Merged
merged 9 commits into from
Jun 3, 2024
Merged

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented May 5, 2024

Closes #440

This is still a bit of a mess, and the new logic is in the wrong spot - it is currently designed to easily update the existing manifests rather than supporting creating new manifests.

Key question atm is whether it is OK to add new fields website & license_markdown to both the base info and manifest structs.

Another is whether it is OK to create a lib.rs ? I moved everything to the lib.rs , but would be happy to move the unmodified parts back to main.rs to reduce the size of the change.

@taiki-e
Copy link
Owner

taiki-e commented May 5, 2024

Thanks!

Key question atm is whether it is OK to add new fields website & license_markdown to both the base info and manifest structs.

I'm okay with adding those as those are small enough information.

Another is whether it is OK to create a lib.rs ? I moved everything to the lib.rs , but would be happy to move the unmodified parts back to main.rs to reduce the size of the change.

I'm okay with either.

@jayvdb
Copy link
Contributor Author

jayvdb commented May 5, 2024

Great.

Next two questions:

  1. Is it ok to generate as TOOLS.md , and link to it from README.md? IMO the list is already getting long. I dont mind merging it into the README.md . It is only a bit more work to do that.

  2. Should the markdown generator be a separate tool , or run at the end of the existing main.rs ?

@taiki-e
Copy link
Owner

taiki-e commented May 5, 2024

Is it ok to generate as TOOLS.md , and link to it from README.md? IMO the list is already getting long. I dont mind merging it into the README.md . It is only a bit more work to do that.

The current list is indeed long, so splitting it into separate files is okay.

Should the markdown generator be a separate tool , or run at the end of the existing main.rs ?

I think the latter is better at this time because it eliminates the need for changes to the CI script.

@jayvdb
Copy link
Contributor Author

jayvdb commented May 5, 2024

IIUC, the CI loops over each tool, in which case putting the markdown generation in the main.rs will mean it does that step many times - once per tool, instead of only once at the end. Anyways, I'll put it in main.rs, but put the logic in lib.rs, so it can become a separate bin later.

@taiki-e
Copy link
Owner

taiki-e commented May 5, 2024

IIUC, the CI loops over each tool, in which case putting the markdown generation in the main.rs will mean it does that step many times - once per tool, instead of only once at the end.

Oh, you are right. That is indeed inefficient and would be better to separate them.

@jayvdb jayvdb force-pushed the gen-readme-table branch 6 times, most recently from 953952b to 6b78595 Compare May 19, 2024 10:49
@jayvdb jayvdb marked this pull request as ready for review May 19, 2024 11:58
@@ -0,0 +1,47 @@
# Tools
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that the new table lost entries that are not managed by manifest from the previous, such as nextest, valgrind.

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've done a comparison now, and it is only nextest and valgrind from README.md that dont appear in TOOLS.md.

Not too hard to have a special case for valgrind.

But, I have also noticed that cargo-watch & watchexec-cli is in

latest) tools+=(cargo-watch watchexec-cli nextest) ;;
major.minor.patch) tools+=([email protected] [email protected] [email protected]) ;;
major.minor) tools+=([email protected] [email protected] [email protected]) ;;
major) tools+=(cargo-watch@8 watchexec-cli@1) ;;
, but these are not in README.md . Any others that you know about ?

cargo-watch can also be a base manifest, getting bins from https://github.com/watchexec/cargo-watch/releases/tag/v8.5.2

watchexec-cli might be the bins at https://github.com/watchexec/watchexec/releases/tag/v2.1.1 - need to double check that one.

I see quite a few other system tools that are installed by https://github.com/taiki-e/install-action/blob/main/main.sh . Should we be mentioning them also? jq, curl, tar also ?

They would be more special cases, which is fine.

Copy link
Owner

Choose a reason for hiding this comment

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

But, I have also noticed that cargo-watch & watchexec-cli is in

tools/ci/tool-list.sh is only for testing. tools/publish.sh is for "Supported tools".

# Not manifest-based
tools+=(valgrind nextest cargo-nextest)

I see quite a few other system tools that are installed by https://github.com/taiki-e/install-action/blob/main/main.sh . Should we be mentioning them also? jq, curl, tar also ?

Let's ignore them at this PR, as they are for incomplete runners and are not usually installed by us.

Comment on lines +27 to +34
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Version {
pub major: Option<u64>,
pub minor: Option<u64>,
pub patch: Option<u64>,
pub pre: semver::Prerelease,
pub build: semver::BuildMetadata,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fwiw, git diff --color-moved is a good way to check changes to the items which were moved to lib.rs.

Or if you like, I can create a separate PR to move them to lib.rs , but it still involves a lot of changes because pub needed to be added to the struct members

tools/codegen/src/main.rs Outdated Show resolved Hide resolved
tools/codegen/src/main.rs Outdated Show resolved Hide resolved
tools/codegen/src/main.rs Outdated Show resolved Hide resolved
tools/codegen/src/main.rs Outdated Show resolved Hide resolved
@@ -561,6 +636,35 @@ fn download_github(url: &str) -> Result<ureq::Response> {
Err(last_error.unwrap().into())
}

#[allow(clippy::missing_panics_doc)]
pub fn github_head(url: &str) -> Result<()> {
eprintln!("fetching head of {url} ..");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this & the eprintln for failure is a bit noisy, especially as the license algorithm expects this to fail quite often as it tries to detect where to find the license file.

@jayvdb jayvdb force-pushed the gen-readme-table branch 4 times, most recently from d83fcc1 to d48fd77 Compare May 28, 2024 00:48
TOOLS.md Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
TOOLS.md Outdated Show resolved Hide resolved
TOOLS.md Outdated Show resolved Hide resolved
TOOLS.md Outdated Show resolved Hide resolved
TOOLS.md Outdated Show resolved Hide resolved
Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @jayvdb!

@taiki-e taiki-e merged commit d7080cb into taiki-e:main Jun 3, 2024
29 checks passed
@jayvdb jayvdb deleted the gen-readme-table branch June 9, 2024 23:16
@taiki-e taiki-e mentioned this pull request Jun 15, 2024
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.

Update table in readme to mention editorconfig-checker
2 participants