Add ⚠️ for deprecated entry #517
Conversation
|
Neat! If merged, you could even trigger this via GitHub Actions, since they can run on a schedule. |
|
First off, wow that's a great idea that hadn't crossed my mind before. The 6 months were picked somewhat arbitrarily and I can see how it might be a bit too harsh for the beginning. All in all, I'd lower the threshold a bit to 2 years for now as this would deprecate quite a few tools in one go already. We could increase the threshold later if we see fit. Alternatively, we could add a new, automated, field to our yaml: What do you both think? |
|
Hey, This is very nice idea. I also think that 6 months is a bit to harsh. For me a year seems good, but i'm fine with 2 years as well. Speaking of adding such a tool to CI pipeline: since readme is generated with rust, maybe we would stick to that language and write a deprecated checked is rust as well? btw, i'd really like to merge this PR before: #484 goes to master. |
|
Hi, Thank you guys for the thoughts! For the criteria of And for the CI part, that was indeed a good suggestion, at least I don't need to run it manually every time to update the status. Currently, I am still very very new to rust, so that's the reason why I wrote python code😂, but I am willing to learn using rust to do the same thing. I tried to google someting about making http request in rust, and I found that there is one thing called reqwest, which seems to be popular for doing this kind of thing. What I have in my mind currently is like this:
Not sure if this make any sense to you guys, please forgive me if I misunderstood anything, any suggestion will be appreciated! BTW, according to this document, calling github api is limited to 60 times per hour for unauthenticated requests. I used my personal authentication token in my python code, but it seems a bit weird if I use it here in rust code, any idea about this? |
Let's go with one year then. 😊
Cool! Your plan sounds good. I'd do the same, basically. We can have a coding session when you have time and I could mentor you if you like. 👍 |
No problem. Thanks a lot for providing those resources, I just spent some time on it, and currently success to use
Thanks a lot in advance, but I prefer to do it by myself first, will let you know if I need help.😄 |
|
Hi, Almost completed, but it seems that Here are part of repos that will fail: I used the sample code here, and filled in my own personal token, then replacing with The error message is What I will do next is to find some other methods to replace |
|
Thanks for the progress report @AristoChen, |
|
Just confirming that it does work from the commandline: yields the correct commits for me. |
|
Just some more observations:
So I guess it could be fixed in a couple of ways:
|
|
Created a bug report here. In the meantime the workaround should be fine. 😊 |
|
BTW this is the sample code for getting the last commit: use anyhow::{anyhow, Result};
use futures::stream::StreamExt;
use hubcaps::{Credentials, Github};
use std::env;
#[tokio::main]
async fn main() -> Result<()> {
let github = Github::new(
String::from("user-agent-name"),
env::var("GITHUB_TOKEN").ok().map(Credentials::Token),
)?;
let last_commit = github
.repo("TNG", "ArchUnit")
.commits()
.iter()
.next()
.await
.ok_or(anyhow!("No commit"));
println!("{:?}", last_commit);
Ok(())
}It does not work and produces the same error. I think hubcaps is deserializing the entire API response and not only the first commit, but I might be wrong. Maybe you find a way to make it work somehow. |
|
In the worst case we can just print the error and ignore these entries for now, e.g. we don't mark them as deprecated. |
|
Hi, Appreciate for all these help! I have tried some other methods in my mind, but no luck to solve the error, so I decided to ignore those entries lol. I still need some time to check the result is correct or not, so I think I will push the commit tomorrow evening, time to sleep in my country haha. BTW, there is one more question that I wanna ask, it's about github token. Currently I wrote this in my code And I notice that your comment here is which seems to cause the rate limit problem, and I am still not sure how to solve this yet. I will try to figure this out after get off work tomorrow, any suggestions will be appreciated! |
|
I'd go with this snippet, which I got from the official documentation: The trick is that you have to set the token as an environment variable like so: After that you can run your program and it will pick up the token from the environment. |
Oh I got you, thanks a lot for this. Still need some more time to check each result🔎 |
|
Hi, Just completed! I added one more thing, I will check whether GITHUB_TOKEN is set or not, if not, then bypass the checking function. I think it is better for other contributors that do not care about Finally, THANKS A LOT for all these suggestions, I believe that there is something that could be handled in a better way, so if you find anything that seems not make sense, feel free to edit it! If possible, let me know what was edited as well, I’d like to know what is the better way 😄 |
data/render/src/lib.rs
Outdated
|
|
||
| let mut entries_tmp: Vec<Entry> = entries.to_vec(); | ||
| for entry in &mut entries_tmp { | ||
| if entry.source.is_some() { |
There was a problem hiding this comment.
I personally prefer a bit more flat structure.
You can restructure this code to negate if condition here and put "continue" in it. That way you'll avoid multiple indentations.
This applies also to the if conditions that you put below.
|
Hi @jakubsacha, Thanks for the suggestion! I edited the code, let me know if there is anything else 👍 |
|
👍 from my side. @mre ? |
data/render/src/lib.rs
Outdated
| let mut source: &str = entry.source.as_ref().unwrap(); | ||
| if source.chars().last().unwrap() == '/' { | ||
| source = source.trim_end_matches('/'); | ||
| } |
There was a problem hiding this comment.
Think those four lines can be replaced with one:
entry.source = entry.source.trim_end_matches('/');The performance/memory overhead is negligible if that was a concern. 😊
There was a problem hiding this comment.
I edited like this, looks a bit long for the first line though
let components: Vec<&str> = entry.source.as_ref().unwrap().trim_end_matches('/').split("/").collect();
if !(components.contains(&"github.com") && components.len() == 5) {
// valid github source must have 5 elements - anything longer and they are probably a
// reference to a path inside a repo, rather than a repo itself.
continue;
}not sure if this make sense?
There was a problem hiding this comment.
You could refactor it a bit and put the extraction of owner and repo into a seprate function:
let (owner, repo) = analyze_repo_url(entry.source)?;Code for the function:
fn analyze_repo_url(url: String) -> Result<(String, String), Box<dyn Error>> {
let components: Vec<&str> = url.trim_end_matches('/').split("/").collect();
if !components.contains(&"github.com") || components.len() != 5 {
// valid github source must have 5 elements - anything longer and they are probably a
// reference to a path inside a repo, rather than a repo itself.
return Err("Cannot extract owner and repo".into());
}
let owner = components[3];
let repo = components[4];
Ok((owner.into(), repo.into()))
}Completely optional, though. 😉
There was a problem hiding this comment.
Oops, I didn't notice this comment before I push the commit
@mre If you prefer this way, I can edit the code and push another commit 😉
data/render/src/lib.rs
Outdated
| Credentials::Token(token), | ||
| )?; | ||
|
|
||
| let mut entries_tmp: Vec<Entry> = entries.to_vec(); |
There was a problem hiding this comment.
Why the temporary allocation here? Ownership issues?
There was a problem hiding this comment.
I knew it looks very weird, I have tried some methods, but using a temporary entries_tmp is the only one that success. I believe there are some better way to handle this.
There was a problem hiding this comment.
Somehow managed to remove the temporary entries_tmp, still not really familiar with handling ownership in rust😵
data/render/src/lib.rs
Outdated
| let commit_list = match commit_list { | ||
| Ok(commit_list) => commit_list, | ||
| Err(_error) => Vec::new(), | ||
| }; |
There was a problem hiding this comment.
No need for this block:
let commit_list = match commit_list {
Ok(commit_list) => commit_list,
Err(_error) => Vec::new(),
};fn main returns a result, so you can simply .await? (note the ?) in the line above.
Good stuff.
There was a problem hiding this comment.
I was originally using await?, but if some repo failed to get commit list, it will only gives me this error message
Error: Codec(Error("invalid type: null, expected struct User", line: 1, column: 67983))
and the program will directly exit, so thats the reason why I use that method to handle this error.
Not sure if there is any other way to handle this?
There was a problem hiding this comment.
Does this make more sense than my original code?
if let Ok(commit_list) = github
.repo(owner, repo)
.commits()
.list()
.await
{
let date = &commit_list[0].commit.author.date;
let last_commit = NaiveDateTime::parse_from_str(&date, "%Y-%m-%dT%H:%M:%SZ")?;
let last_commit_utc = DateTime::<Utc>::from_utc(last_commit, Utc);
let duration = Local::today().signed_duration_since(last_commit_utc.date());
if duration.num_days() > 365 {
entry.deprecated = Some(true);
} else {
entry.deprecated = None;
}
}
data/render/src/lib.rs
Outdated
| }; | ||
| if commit_list.len() == 0 { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
No need for that check either. It will be redundant once we use the ? above. (See previous comment.)
|
Hi, THANKS A LOT for all those suggestions! Just completed the modification, please let me know if there is anything else 😄 |
data/render/src/lib.rs
Outdated
| @@ -21,6 +23,50 @@ pub fn validate(tags: &Tags, entries: &Vec<Entry>) -> Result<(), Box<dyn Error>> | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[tokio::main] | |||
| pub async fn check_deprecated(token: std::string::String, entries: &mut Vec<Entry>) -> Result<(), Box<dyn Error>> { | |||
There was a problem hiding this comment.
std::string::String is part of the Rust prelude, which is a fancy way of saying you can just use String here without importing anything. 😎
|
Cool beans! Just mentioned one final but I think we're ready to merge. 👍 Feel free to push the big green button. 🕺 |
|
Let me take a moment to say that I really love the collaborative spirit around this project. Thanks for all the work from everybody. ⭐ |
|
Hi, I replaced If there is nothing else, I will click the big green button to merge PR later 😄 THANKS A LOT for all the help and suggestions, really glad to have this opportunity to collaborate with you guys! |
Same! Thanks for pushing through. Ready to merge. 😊 |
Hi,
I think this commit change lots of entry, so I guess I should create a PR instead of push to upstream directly.
So here is the story, I wrote a python code(still not a rust expert 😂) to iterate through all entry, and use github api to check the date for latest commit. I found that there are 102 github repos that was not updated for more than 6 months(I use 180 days as criteria in my code currently), which meets the criteria of
deprecatedHere is the list:
so maybe I will run my python code every month to update the
deprecatedstatus from now on, let me know if have any thoughts, Thanks :)