leogdion / PackageListValidator

A tool for validating the SwiftPM Library/Swift Package Index master package list.
MIT License
1 stars 0 forks source link

Merging into the PackageList repository #13

Closed daveverwer closed 4 years ago

daveverwer commented 4 years ago

First of all, thanks so much for your work here @leogdion. I'm sorry I've been so neglectful on this project over the last couple of weeks.

I'm making a thread here rather than in the PR as I don't think we're quite ready to start using the new validator yet. I had a chat with Sven about it this morning, and these were points raised in the conversation:

  1. When you and I talked about making this a tool, I assumed the PackageList repository would ship with a binary rather than using swift-sh to pull it in dynamically. As we're asking people to run the tool before submitting a PR, it needs to be trivial to run. I think it needs to have no dependencies. @finestructure also worried that there might be a problem shipping a command-line tool that relies on the presence of the Swift runtime, but I'll let Sven chip in on that.
  2. On the same issue of dependencies, we should not depend on a GitHub token. It's no problem for it to support a token, but it should not require one. Most diff invocations will only be for one package, and those that are for more are rarely more than a few. GitHub allows up to 60 requests/hour without a token, which should be enough for all but the most extreme cases.
  3. I've mentioned this a few times, but I think it's worth saying again. If all is causing this to become more complex than it needs to be, we should drop support for it. It was vital when we were trying to increase the quality of the list, but now it's in good shape, and everything is validated as it's added, I think we could simplify this significantly by dropping all support.

I'm sorry this didn't end in a simple merge, but I think getting it right is important.

leogdion commented 4 years ago
  1. When you and I talked about making this a tool, I assumed the PackageList repository would ship with a binary rather than using swift-sh to pull it in dynamically. As we're asking people to run the tool before submitting a PR, it needs to be trivial to run. I think it needs to have no dependencies. @finestructure also worried that there might be a problem shipping a command-line tool that relies on the presence of the Swift runtime, but I'll let Sven chip in on that.

Yeah no problem. It actually is a binary (i.e. command-line tool). That's why I suggested using mint. I just wasn't sure about how to deliver the app? Mint install, Swift dependencies? It does compile to a command-line tool.

  1. On the same issue of dependencies, we should not depend on a GitHub token. It's no problem for it to support a token, but it should not require one. Most diff invocations will only be for one package, and those that are for more are rarely more than a few. GitHub allows up to 60 requests/hour without a token, which should be enough for all but the most extreme cases.

I'll change the code so it doesn't require the token. That makes sense.

  1. I've mentioned this a few times, but I think it's worth saying again. If all is causing this to become more complex than it needs to be, we should drop support for it. It was vital when we were trying to increase the quality of the list, but now it's in good shape, and everything is validated as it's added, I think we could simplify this significantly by dropping all support.

Yeah I totally agree. Would you like me to just remove documentation for it but keep the functionality or completely remove that subcommand?

daveverwer commented 4 years ago

Yeah no problem. It actually is a binary (i.e. command-line tool). That's why I suggested using mint. I just wasn't sure about how to deliver the app? Mint install, Swift dependencies? It does compile to a command-line tool.

The way I envisioned it was that we'd ship a binary, maybe called validate so that the command to execute it would still simply be ./validate diff.

I'll change the code so it doesn't require the token. That makes sense.

👍

Yeah I totally agree. Would you like me to just remove documentation for it but keep the functionality or completely remove that subcommand?

It's up to you really. There's no need to remove the code just for the sake of it, but at the same time if that allows a simplification it mniht be worthwhile? I'm easy on it, but I agree we should get rid of the documentation for it.

leogdion commented 4 years ago

The way I envisioned it was that we'd ship a binary, maybe called validate so that the command to execute it would still simply be ./validate diff.

There's a binary there swiftpmls I named it that because validate seemed vague in a larger context. I had suggested having folks install the binary via mint since it's basically a swift package underneath but I am open to suggestions. I just need some clarification on what shipping a binary.

It's up to you really. There's no need to remove the code just for the sake of it, but at the same time if that allows a simplification it mniht be worthwhile? I'm easy on it, but I agree we should get rid of the documentation for it.

I say just remove the documentation. I think as imperfect as it it'd be nice to keep around for us.

daveverwer commented 4 years ago

I see a swiftpmls directory, with a main.swift in it but no binary. But I'd not expect to see a binary in this repository, I'd only expect to see a binary in the PackageList repository.

I just need some clarification on what shipping a binary.

To me, it means making a binary file with swiftc and committing that binary file into the PackageList repository.

By the way, what does swiftpmls stand for?

leogdion commented 4 years ago

Thanks that helps a lot. I'll move forward with the changes stated above and then add the binary to the repo.

swiftpmls stands for Swift Package Manager LiSt :)

leogdion commented 4 years ago

All issues have been resolved. Please let me know if you have any other questions or concerns.

daveverwer commented 4 years ago

So we're at a stage where we could add a validate binary into the other repo as a PR? Let's do that.

Let's call it validate as we add it to the Package List repository. I think that's a better name than swiftpmls in that context.

leogdion commented 4 years ago

Done.

daveverwer commented 4 years ago

Closed by PackageList:#433.