feat: add shield badges in README.md #96

Angel-Dijoux · 2024-11-04T17:17:37Z

feat: implement shield badge

feat: inject score badge in readme after test

chore: fix generated badge md

jpopesculian

Looking good! Generally, I would split features up into separate stories, even if they're small. These two features aren't related at all to each other so they shouldn't be dependent on each other

jpopesculian · 2024-11-05T10:47:24Z

src/commands/new/git.rs

@@ -0,0 +1,20 @@
+use git2::{Error, Repository, RepositoryInitOptions};


this shouldn't be in commands/new/git. otherwise we would expect there to be aqora new git. Just put it in the src for now. At some point when things get too messy we'll do a little reorganization. But also this should apply to aqora template as well as aqora new

jpopesculian · 2024-11-05T10:49:39Z

src/commands/test.rs

@@ -345,6 +366,35 @@ pub async fn run_submission_tests(
                "Score".if_supports_color(OwoStream::Stdout, |text| { text.bold() }),
                score
            ));
+            let path = global.project.join("README.md");


I think you'll have to locate the readme based on the pyproject. see https://docs.rs/pyproject-toml/latest/pyproject_toml/struct.Project.html#structfield.readme

jpopesculian · 2024-11-05T10:52:23Z

src/commands/test.rs

+                        let mut contents = String::new();
+                        file.read_to_string(&mut contents).await?;
+                        file.seek(SeekFrom::Start(0)).await?;
+                        file.write_all(


I'm not sure I understand how this would replace an existing badge? I think you'd have to wrap it in some comments or parse the markdown to find the existing badge

No! I want to inject a new badge each time with the latest score the user achieved! So basically, we have two types of badges:

A default use-case badge, which redirects to the Aqora use case.

A score badge that displays the user’s score!

Maybe it’s not ideal to keep all badges? I thought they could display either progression or regression, but users can always edit the markdown themselves at the end if they want to remove some of them!

I would say we should only have one badge generated at a time. thoughts @stubbi ?

Oh no, I didn't express myself clearly. There is only one generated badge. However, in the markdown before any test, there is already a badge that refers to Aqora. Then, when you run a test, a new badge with a score is generated, and each test adds a new score badge!

template/assets/use_case/README.md.hbs

jpopesculian

looking good! just a few things

jpopesculian · 2024-11-12T16:45:42Z

src/commands/new/use_case.rs

    pb.finish_with_message(format!(
        "Created use case in directory '{}'",
        dest.display()
    ));
    Ok(())
 }
+
+fn format_permission_error(action: &str, dest: &Path, error: &impl std::fmt::Display) -> Error {


its kind of strange to create a function for this kind of thing, if you're only going to use it once

jpopesculian · 2024-11-12T16:46:02Z

Cargo.toml

@@ -46,7 +46,7 @@ mime = "0.3"
 open = "5.0"
 owo-colors = { version = "4.0", features = ["supports-colors"] }
 passterm = "2.0"
-pyo3 = { version = "0.20", features = ["serde"] }
+pyo3 = { version = "0.20", features = ["serde"]}


haha this is the ultimate nitpick but this really bugs me

jpopesculian · 2024-11-12T16:52:12Z

src/commands/test.rs

+                let mut contents = String::new();
+                file.read_to_string(&mut contents).await?;
+                let updated_content = regex_replace_all!(
+                    r"(?:<!--- score_badge -->)|(!\[Aqora Score Badge\]\([^\)]+\))",


this would probably but i think its usually clearer to start and end tags on things, and also to properly format your comments e.g.

.*

jpopesculian · 2024-11-13T15:01:16Z

src/commands/test.rs

@@ -325,6 +387,7 @@ pub async fn run_submission_tests(
        })
        .collect::<Result<Vec<_>>>()?;

+    let competition = modified_use_case.clone().competition.unwrap_or_default();


nitpick: can you move this either towards the top where we do all of the parsing or closer to where you use it? also i'm not sure if an empty string is the best way to handle None. I think the badge function would probably decide whether to send them to the aqora homepage or to the competition

jpopesculian · 2024-11-13T15:02:10Z

src/commands/test.rs

@@ -345,6 +408,14 @@ pub async fn run_submission_tests(
                "Score".if_supports_color(OwoStream::Stdout, |text| { text.bold() }),
                score
            ));
+            let path = global.project.join(project_readme_path.ok_or_else(|| {


i don't think the README should be required. I would just skip the badge function if it doesn't exist

jpopesculian · 2024-11-13T15:03:13Z

src/commands/test.rs

+                    "See : https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#readme",
+                )
+            })?);
+            update_score_badge_in_file(path, &competition, &score, global.aqora_url().unwrap())


I would avoid using unwrap if its not necessary. should be able to just do ?

jpopesculian · 2024-11-13T15:06:17Z

src/commands/test.rs

+        .and_then(|readme| match readme {
+            aqora_config::ReadMe::RelativePath(path) => Some(path),
+            aqora_config::ReadMe::Table { file, .. } => file.as_ref(),
+        });


check crate::readme for how we look for the readme. maybe you can break up that function to providing the path and reading it

template/assets/use_case/template/README.md.hbs

jpopesculian

Looking better!!!

jpopesculian · 2024-11-22T14:52:29Z

src/readme.rs

-            break;
+
+    if let Some(path) = path {
+        return Ok(Some(path));


This path is relative so it has to be joined to the project_dir

jpopesculian · 2024-11-22T14:53:57Z

src/readme.rs

@@ -1,82 +1,85 @@
 use aqora_config::ReadMe;


I'm not sure if these updates are 100% correct. You can pass a README inline directly into the pyproject.toml. If you want to explicitly not allow those types of README, you have to throw a relevant error because its not very transparent here. I think for 100% correctness, we would have 2 functions. One for reading the README like we had before, and one for writing the README, where we either get the path and write to the file or we use toml_edit to write to [readme.text]

jpopesculian · 2024-11-22T14:55:48Z

src/commands/test.rs

+                    &score,
+                    global.aqora_url()?,
+                )
+                .await?;


I would maybe just emit a warning here if we can't write to the README as it would be
a shame to throw an error for something that's decorative and make people feel like they need to rerun their tests

jpopesculian · 2024-11-22T14:56:25Z

src/commands/test.rs

+        project.project.as_ref().and_then(|p| p.readme.as_ref()),
+    )
+    .await
+    .map_err(|err| {


I'd throw a warning here as its just decorative / a nice to have

jpopesculian · 2024-11-22T14:59:20Z

src/commands/test.rs

+                let mut contents = String::new();
+                file.read_to_string(&mut contents).await?;
+                let updated_content = regex_replace_all!(
+                    r"(?:<!--\s*aqora:score:start\s*-->.*?<!--\s*aqora:score:end\s*-->)|(!\[Aqora Score Badge\]\([^\)]+\))",


Would this replace ![Aqora Score Badge](...) with ![Aqora Score Badge](...) or am I reading this wrong? Shouldn't it wrap the the replaced section in comments as well?

Nope! This is going to replace  or ![Aqora Score Badge](...)with the generated badge. The thing is, we have to either comment out or replace an existing badge that we don’t want to keep!

feat: implement shield badge feat: inject score badge in readme after test chore: fix generated badge md

stubbi · 2024-12-17T21:13:24Z

@Angel-Dijoux what's the state of this one?

Angel-Dijoux · 2024-12-18T09:29:46Z

@Angel-Dijoux what's the state of this one?

Waiting for @volgar1x with the badge renderer server (https://github.com/aqora-io/deno-badge-renderer)

volgar1x · 2024-12-18T11:38:08Z

@Angel-Dijoux what's the state of this one?

Waiting for @volgar1x with the badge renderer server (https://github.com/aqora-io/deno-badge-renderer)

I wasn't aware you were blocked sorry!

Have you updated this pr to work with this new server?

Angel-Dijoux self-assigned this Nov 4, 2024

Angel-Dijoux force-pushed the angel/add_git_default_template branch from 41fcb6f to abd287e Compare November 4, 2024 17:19

Angel-Dijoux requested review from jpopesculian and volgar1x November 4, 2024 17:19

jpopesculian requested changes Nov 5, 2024

View reviewed changes

Angel-Dijoux changed the title ~~feat: init a repo for default use_case template~~ feat: add shield badges in README.md Nov 5, 2024

Angel-Dijoux force-pushed the angel/add_git_default_template branch 2 times, most recently from 2f3d59d to a2ff781 Compare November 5, 2024 18:44

Angel-Dijoux requested a review from jpopesculian November 9, 2024 10:42

jpopesculian requested changes Nov 13, 2024

View reviewed changes

Angel-Dijoux force-pushed the angel/add_git_default_template branch from 4b1ec44 to 8e2d5f2 Compare November 15, 2024 18:07

Angel-Dijoux requested a review from jpopesculian November 15, 2024 18:07

Angel-Dijoux enabled auto-merge November 20, 2024 16:24

jpopesculian requested changes Nov 22, 2024

View reviewed changes

Angel-Dijoux added 9 commits November 26, 2024 22:23

feat: init a repo for default use_case template

bde968f

fix: pr suggestions

a1681cf

feat: init a repo for default use_case template

45683a9

feat: implement shield badge feat: inject score badge in readme after test chore: fix generated badge md

ci: fix build issue

107d2a5

fix: use readme from pyproject.toml

c212d01

fix: use readme from pyproject.toml

ff8b1eb

chore: remove git2 from here

e608bca

fix: pr suggesstions

c60b97d

chore: remove tests files

9746408

Angel-Dijoux force-pushed the angel/add_git_default_template branch from a244436 to c011131 Compare November 26, 2024 21:24

chore: move write op for readme in readme crate

a37dbfa

Angel-Dijoux force-pushed the angel/add_git_default_template branch from c011131 to a37dbfa Compare November 27, 2024 07:55

Angel-Dijoux requested a review from jpopesculian November 27, 2024 07:55

Angel-Dijoux marked this pull request as draft December 6, 2024 18:41

auto-merge was automatically disabled December 6, 2024 18:41
Pull request was converted to draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add shield badges in README.md #96

feat: add shield badges in README.md #96

Angel-Dijoux commented Nov 4, 2024

jpopesculian left a comment

jpopesculian Nov 5, 2024

jpopesculian Nov 5, 2024

jpopesculian Nov 5, 2024

Angel-Dijoux Nov 5, 2024

Angel-Dijoux Nov 5, 2024

jpopesculian Nov 5, 2024

Angel-Dijoux Nov 5, 2024

jpopesculian left a comment

jpopesculian Nov 12, 2024

jpopesculian Nov 12, 2024

jpopesculian Nov 12, 2024

jpopesculian Nov 13, 2024

jpopesculian Nov 13, 2024

jpopesculian Nov 13, 2024

jpopesculian Nov 13, 2024

jpopesculian left a comment

jpopesculian Nov 22, 2024

jpopesculian Nov 22, 2024

jpopesculian Nov 22, 2024

jpopesculian Nov 22, 2024

jpopesculian Nov 22, 2024

Angel-Dijoux Nov 26, 2024

stubbi commented Dec 17, 2024

Angel-Dijoux commented Dec 18, 2024

volgar1x commented Dec 18, 2024

		@@ -0,0 +1,20 @@
		use git2::{Error, Repository, RepositoryInitOptions};

feat: add shield badges in README.md #96

Are you sure you want to change the base?

feat: add shield badges in README.md #96

Conversation

Angel-Dijoux commented Nov 4, 2024

jpopesculian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpopesculian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpopesculian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stubbi commented Dec 17, 2024

Angel-Dijoux commented Dec 18, 2024

volgar1x commented Dec 18, 2024