Opened 3 years ago

Closed 3 years ago

#27400 closed enhancement (fixed)

minor details in mac-app

Reported by: chapoton Owned by:
Priority: minor Milestone: sage-8.7
Component: distribution Keywords:
Cc: embray, jdemeyer, jhpalmieri, tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw, John Palmieri
Report Upstream: N/A Work issues:
Branch: 472fde3 (Commits, GitHub, GitLab) Commit: 472fde395710f02b366f134ce0c8bf0cdab4c3a3
Dependencies: Stopgaps:

Status badges

Description

adding missing brackets, plus some cosmetics

Change History (17)

comment:1 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/27400
  • Commit set to 55387107251a094ed529926ce5905658a62021f8
  • Status changed from new to needs_review

New commits:

5538710minor changes in mac-app

comment:2 Changed 3 years ago by kcrisman

I'm not sure that we need to remove all such contractions. They are a standard part of English, and in this case may actually make the quotes from 'famous people' wrong (if they are, in fact, legitimate quotations!). Is there a style guide I'm unfamiliar with you've been following for doing this? It's true (I mean, IT IS true) that separating them out can be helpful in some contexts, but such diction actually will seem stilted, if used consistently.

comment:3 follow-up: Changed 3 years ago by chapoton

Ok, I take good note of your worries about contractions.

But the main point of the ticket is the addition of {}, for otherwise the code is plain wrong.

comment:4 Changed 3 years ago by git

  • Commit changed from 55387107251a094ed529926ce5905658a62021f8 to f895905cb2f019488bac9e77ec4ab0d52ff02422

Branch pushed to git repo; I updated commit sha1. New commits:

b89f7dcMerge branch 'u/chapoton/27400' in 8.7.b6
f895905fix the citations ; turn some http to https

comment:5 Changed 3 years ago by chapoton

ok, done

comment:6 Changed 3 years ago by chapoton

  • Cc tscrim added

ready for review, please

comment:7 in reply to: ↑ 3 Changed 3 years ago by kcrisman

But the main point of the ticket is the addition of {}, for otherwise the code is plain wrong.

I believe you, but I don't know quite enough about that part to review it properly (since the app works already). Hopefully Travis can give it a quick review; I don't have a real problem with anything of course!

comment:8 follow-up: Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

The code clearly indicates that it should be grouped under that if statement, so I am fine with that change (although the alignment spacing looks a little odd, but I am not going to hold a positive review up for that).

I would argue the reason to avoid contractions is this is "formal" writing, where you should not use them (or so I've been told by my paper referees when I accidentally have one slip in).

Although I cannot test that this does work (I do not mac to test it on). If someone (Karl-Dieter) can verify that this works after the change, then we can flip it to positive.

comment:9 Changed 3 years ago by kcrisman

Unfortunately I don't have time to test this out in the immediate future.

comment:10 in reply to: ↑ 8 Changed 3 years ago by jhpalmieri

Replying to tscrim:

The code clearly indicates that it should be grouped under that if statement, so I am fine with that change (although the alignment spacing looks a little odd, but I am not going to hold a positive review up for that).

The spacing is because the original file has lots of tabs in it, and with the branch, some of those tabs have been replaced by spaces. It's an html file, so it shouldn't matter, I think.

I would argue the reason to avoid contractions is this is "formal" writing, where you should not use them (or so I've been told by my paper referees when I accidentally have one slip in).

In this case we're quoting people (supposedly) so we should use their original words: if they used contractions, so should we. I'm too lazy to actually search for the original quotes to verify. (I'm really just echoing what kcrisman said already.)

Although I cannot test that this does work (I do not mac to test it on). If someone (Karl-Dieter) can verify that this works after the change, then we can flip it to positive.

I'll try to test it and report back.

comment:11 Changed 3 years ago by jhpalmieri

First, the main functionality of the notebook works with or without this change. With the branch here, there is minor breakage: the app opens up a page in the web browser which says at the bottom, "Show a random tip. Show all." and then a random tip. Without this branch, you can click on "Show a random tip" and the tip changes. With this branch, it shows the first tip in the file and does not change. Remove the braces {} and it works again.

comment:12 Changed 3 years ago by git

  • Commit changed from f895905cb2f019488bac9e77ec4ab0d52ff02422 to 472fde395710f02b366f134ce0c8bf0cdab4c3a3

Branch pushed to git repo; I updated commit sha1. New commits:

472fde3remove the added brace, but change the indend

comment:13 Changed 3 years ago by chapoton

Thanks. So the original behaviour was the intended one. I removed the {}, but instead changed the original indent to make clear that the second line is not inside the if block.

comment:14 Changed 3 years ago by chapoton

Now really a trivial ticket..

comment:15 Changed 3 years ago by jhpalmieri

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, John Palmieri
  • Status changed from needs_review to positive_review

comment:16 Changed 3 years ago by jhpalmieri

This works.

comment:17 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/27400 to 472fde395710f02b366f134ce0c8bf0cdab4c3a3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.