search

stardot / beebasm

A portable 6502 assembler with BBC Micro style syntax
http://www.retrosoftware.co.uk/wiki/index.php/BeebAsm
GNU General Public License v3.0
84 stars 26 forks source link

-writes should be -cycle #74

Closed jgharston closed 2 years ago

jgharston commented 2 years ago

I was wondering what the "sets the write value" meant and dug through the documentation.

Ah, it sets the disk cycle. That's what it's called, you should specify it by what it's actually called, as with other tools, and to match the documentated nomenclature, eg https://mdfs.net/Docs/Comp/Disk/Format/DFS

chriskillpack commented 2 years ago

@ZornsLemma WDYT?

ZornsLemma commented 2 years ago

It looks like tricky's happy to change it (https://stardot.org.uk/forums/viewtopic.php?p=368483#p368483) and "cycle" seems a decent name to me. Do you want me to tweak this on proposed-updates?

ZornsLemma commented 2 years ago

I've done this on a branch for now: https://github.com/stardot/beebasm/tree/issue-74

chriskillpack commented 2 years ago

Want to make this a PR and then link to the issue? Once the PR is merged into proposed-updates it will automatically close this issue.

BTW Why does your commit message include a copy of the diff output?

ZornsLemma commented 2 years ago

I don't know why the diff output is there; I did "git commit -av" but I nearly always do that and the man page says that diff doesn't get included in the commit message. My best guess is that I accidentally deleted a blank line or something. I'll create a new issue-74b branch with that fixed and do a pull request for that.

ZornsLemma commented 2 years ago

Right, I think I got there in the end: https://github.com/stardot/beebasm/pull/77

I don't know if we should delete the issue-74 and issue-74b branches with the bad commit message on - what do you think? issue-74c is the one mentioned in the pull request.

mungre commented 2 years ago

The change looks good. Thanks for doing this.

I'd definitely delete the duff branches. I've also deleted the source branches for the last couple of PRs I merged because the list of branches was getting rather long, but that's not a great rationale.

chriskillpack commented 2 years ago

I merged it in, thanks for doing it.

I asked about diff output in commit message because that has happened to me before, but only when I was making changes in a repo where the authors primarily worked in Windows (e.g. CR/LF) and I was making changes on a Mac. I'm going to blame a combination of me not checking, my editor and finally git.

chriskillpack commented 2 years ago

Closing this issue. We will include this in Release Candidate 2.

ZornsLemma commented 2 years ago

Thanks. Just to note I've just deleted the duff issue-74 and issue-74b branches (with the bad commit message; the code change was fine). I've left issue-74c (the one that actually got merged), as mungre says maybe we should clean up more branches later but I didn't want to get too carried away right now.

I am on Linux and it's vaguely possible line-ending stuff played a role in the diff getting include in the commit message. I'll try to see if I can spot a pattern if it happens again...