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

More readme fixes #430

Merged
merged 8 commits into from
Nov 1, 2022
Merged

More readme fixes #430

merged 8 commits into from
Nov 1, 2022

Conversation

@AbdulrhmnGhanem AbdulrhmnGhanem added this to the alpha-2 milestone Sep 28, 2022
@AbdulrhmnGhanem AbdulrhmnGhanem temporarily deployed to staging September 28, 2022 15:06 Inactive
@AbdulrhmnGhanem AbdulrhmnGhanem temporarily deployed to staging September 28, 2022 19:59 Inactive
@AbdulrhmnGhanem AbdulrhmnGhanem temporarily deployed to staging September 28, 2022 20:15 Inactive
@AbdulrhmnGhanem AbdulrhmnGhanem marked this pull request as ready for review October 3, 2022 11:52
@kasbah
Copy link
Member

kasbah commented Oct 5, 2022

Can we add some integration (or maybe even unit tests!?) for this functionality?

@AbdulrhmnGhanem
Copy link
Member Author

I added e2e tests for them and will move them to the processor later as part of #448.

Copy link
Member

@kasbah kasbah left a comment

Choose a reason for hiding this comment

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

Looks good, minor suggestions again. Please also correct the spelling mistakes in your commit messages.

e2e/cypress/integration/readme.spec.js Outdated Show resolved Hide resolved
e2e/cypress/integration/readme.spec.js Outdated Show resolved Hide resolved
processor/src/utils.ts Outdated Show resolved Hide resolved
@kasbah
Copy link
Member

kasbah commented Oct 26, 2022

I pushed some refactoring to this branch because I was working of off the review branch and I couldn't resist. Should have caught this in earlier reviews but linkifyKitspaceYaml had some issues around mutation of the input and also doing so off in an async way after the function returned. Wouldn't have caused any issues yet but it's a bit of a footgun down the line.

@AbdulrhmnGhanem
Copy link
Member Author

mutation of the input and also doing so off in an async way after the function returned.

Do you mean this line?

 Object.keys(kitspaceYaml.multi).forEach(async subProject => {
   linkifiedKitspaceYaml.multi[subProject] = await renderProjectSummary(kitspaceYaml.multi[subProject])
 })

@kasbah
Copy link
Member

kasbah commented Oct 27, 2022

Yeah, that creates an async function for each key in the yaml but doesn't await or return it. Just before that you had

const linkifiedKitspaceYaml = kitspaceYaml

Which references the same object and then you mutate it with the async functions.

The scenario where this could have caused problems is if we had another sub-task that shares the kitspaceYaml with writeKitspaceYaml and wanted to use the summary for something where it really shouldn't be html.

Overall I'm wondering if this is really the right place to render the description. Shouldn't the frontend just handle it? It's fine for now... let's not spend time on it but something to think about for the future.

@kasbah
Copy link
Member

kasbah commented Oct 27, 2022

s/format in project reamde & summary/format in project readme & summary/

@AbdulrhmnGhanem AbdulrhmnGhanem force-pushed the readme-processing-2 branch 4 times, most recently from d56c393 to 73b67b2 Compare October 27, 2022 17:35
@AbdulrhmnGhanem AbdulrhmnGhanem marked this pull request as draft October 27, 2022 18:26
- The repo gets cloned before initializing the value for
  `default_branch` this can cause the jobs to get created without
  `default_branch`.
@kasbah kasbah marked this pull request as ready for review November 1, 2022 17:04
@kasbah kasbah merged commit d3434d2 into master Nov 1, 2022
@AbdulrhmnGhanem AbdulrhmnGhanem deleted the readme-processing-2 branch November 17, 2022 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants