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

Issue 1268 #1304

Closed
wants to merge 4 commits into from
Closed

Issue 1268 #1304

wants to merge 4 commits into from

Conversation

mfirry
Copy link
Contributor

@mfirry mfirry commented Nov 21, 2023

Fixes #1268

@mfirry
Copy link
Contributor Author

mfirry commented Nov 21, 2023

I messed it up with commit messages... sorry! :)

Copy link
Member

@adpi2 adpi2 left a comment

Choose a reason for hiding this comment

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

Here are a few suggestions but overall it looks good.

For the commit message I would appreciate if you could squash all commits into a single one.

Future.successful("No README found for this project, please check the repository")
}
}
.flatMap(res => Future { element.innerHTML = res })
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.flatMap(res => Future { element.innerHTML = res })
.map(res => element.innerHTML = res)

Comment on lines +105 to +109
if (res.status == 200) {
res.text().toFuture
} else {
Future.successful("{\"default_branch\": \"master\"}")
}
Copy link
Member

Choose a reason for hiding this comment

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

`
For the fallback, I would avoid creating a JSON string if we then need to parse it and extract one field.

Suggested change
if (res.status == 200) {
res.text().toFuture
} else {
Future.successful("{\"default_branch\": \"master\"}")
}
def extractDefaultBranch(text: String): String =
js.JSON.parse(text).asInstanceOf[Repo].default_branch
if (res.status == 200) res.text().map(extractDefaultBranch).toFuture
else Future.successful("master")

Comment on lines +64 to +65
def setLogo(): Future[Unit] = {
def getLogoAndSetHTML(branch: String, organization: String, repository: String): Unit = {
Copy link
Member

Choose a reason for hiding this comment

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

It does not just set the logo, it fixes all the URLs of all the images in the readme file.

Suggested change
def setLogo(): Future[Unit] = {
def getLogoAndSetHTML(branch: String, organization: String, repository: String): Unit = {
def getDefaultBranchAndFixImages(): Future[Unit] = {
def fixImages(branch: String, organization: String, repository: String): Unit = {

@mfirry mfirry closed this Nov 22, 2023
@mfirry mfirry deleted the issue-1268 branch November 22, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] image in readme would crash when default branch is not master
2 participants