-
Notifications
You must be signed in to change notification settings - Fork 281
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
Switch to gengo #1152
base: main
Are you sure you want to change the base?
Switch to gengo #1152
Conversation
These are currently combined with Shell.
These are combined with JavaScript and TypeScript.
Gengo will report the type (category) of the detected language.
Should the match language {
"C++" => c_plus_plus_ascii
} The other option is to make language a wrapper for |
Hey, looks like the language titles in our vercel deployment are prettier now 🎉 |
9779742
to
c6913f3
Compare
@o2sh How much do we care about breaking changes in the Like this: Oldimpl Language {
pub fn get_ascii_art(&self) -> &'static str {
// ...
}
} Newpub(crate) get_ascii_art(s: &str) -> &'static str {
// ...
} |
No one seems to be using onefetch as a dependency (https://crates.io/crates/onefetch/reverse_dependencies), so we can do whatever we want 😅
Is there a reason why |
Yeah, I went back and forth a bit on this one. If you look in the commit history when I was initializing the project, you'll see that I did start of with an enum, building out Rust code with tera like onefetch and tokei does. I also experimented with a macro. But after thinking about it a bit, I chose
But mostly, and this might be a bias from taking a break from Rust for a while, but a branch for every single language honestly just seemed like overkill. With several methods on an enum that each match on themselves to return a literal, it just kind of felt like an array of structs with extra steps 😆 In terms of testing: I recognize that coverage on its own isn't indicative of quality, but I don't really like a pattern where a simple addition to a data file will raise or lower test coverage. It's kind of cheating when you get +90% coverage only because you have 100+ branches and a repetitive test for each one 😆 Though, to be fair, all those tests would certainly guarantee that a language will never be removed. |
@@ -17,7 +16,6 @@ Abap: | |||
- "#EEEEEE" | |||
chip: "#E8274B" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the chip color
could also be given by gengo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, we need it for the vercel UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if that's the only reason to keep it, we might as well remove it 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sounds good to me. I didn't add it initially since gengo's colors are different (onefetch's are all from linguist, right?)
Guess we could still get gengo's colors into the vercel UI with WASM, but that feels like major overkill 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just how I prefer how PRs/diffs are made, but I'm thinking we could remove chip colors and use gengo's in a separate PR.
Makes sense. It's just unfortunate that this makes the interface with onefetch somewhat awkward. Additionally, we might encounter runtime errors if we misspell a language name. But I agree, that if we decide to take this route, we might as well drop the |
🤔 With tokei we had to reference the docs or |
93d59ec
to
4df4e08
Compare
@o2sh Looping you in on some conversations over at gengo (spenserblack/gengo#164). Gengo can now analyze submodules (thanks @Byron!), but they are excluded from statistics by default (marked as vendored). In fact, they're kind of a special case where they're always vendored. So a couple of questions since onefetch is a potential user of gengo as a library:
|
Cargo.toml
Outdated
@@ -37,6 +37,7 @@ bytecount = "0.6.3" | |||
clap.workspace = true | |||
clap_complete = "4.3.2" | |||
crossbeam-channel = "0.5.8" | |||
gengo = "^0.5.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that gengo
is also using gix
it's worth noting that both need to coordinate their gix
versions while pre-1.0. Otherwise you would end up with two distinct gix
-dependency trees which costs double to compile and in the final binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would want to create a group in the dependabot config so that gix
and gengo
are updated simultaneously. On gengo's side I can try to promptly create a new release for significant gix
updates.
Sorry for the late answer @spenserblack
It looks like |
In preparation of #1166.
Merge remote-tracking branch 'upstream/main' into chore/26/switch-to-gengo
|
Heads up that the latest version of gengo supports analyzing directories. You can play with it and compare performance between a fully-generic implementation that analyzes the filesystem vs. a git-specific implementation that analyzes git objects. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Just want to point out that analyzing a git history can make some fun tricks available. For example, building a history of language info: # this example is for the gengo executable, but could easily be converted to a
# onefetch version.
for rev in $(git log --reverse --format="%H"); do
gengo git --rev "$rev"
echo # add a newline
done |
v0.11 Now uses an |
This makes some changes to our language support, which I'll be happy to discuss.
Some notable things:
IDK about the gix side, butgengo supports bare repos, so maybe we can remove that error messageTo Do
Language
andtokei::LanguageType
enums.Resolves #26