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

J & K variables #166

Merged
merged 9 commits into from Nov 3, 2018
Merged

J & K variables #166

merged 9 commits into from Nov 3, 2018

Conversation

alpha-cactus
Copy link
Contributor

What does this PR do?

adds J / K variables that are local to each script. similar to I each script gets it's own J / K variable, however it does not inherit the calling script value for J / K unlike I.

Provide links to any related discussion on lines.

https://llllllll.co/t/teletype-3-feature-requests-and-discussion/16219/91?u=alphacactus

How should this be manually tested?

tested by setting J / K variable values in all scripts. testing included calling scripts then setting values, incrementing/decrementing on consecutive calls, modifying in delayed commands, using the INIT script to init values, and calling and returning from other scenes using the SCENE op.

Any background context you want to provide?

If the related Github issues aren't referenced in your commits, please link to them here.

note that make format never modified any of the files I changed. unsure if my formatting was sufficient enough, or if there was an issue with the command.

I have,

  • updated CHANGELOG.md
  • updated the documentation
  • run make format on each commit

src/ops/init.c Outdated
if (v >= 0 && v < TEMP_SCRIPT) ss_clear_script(ss, (size_t)v);
if (v >= 0 && v < TEMP_SCRIPT) {
ss_clear_script(ss, (size_t)v);
ss->variables.j[v] = 0;
Copy link
Member

Choose a reason for hiding this comment

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

maybe move this and the next line to ss_clear_script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@scanner-darkly
Copy link
Member

should also add J and K description to help (help_mode.c).

other than that looks good, thanks for doing this!

@alpha-cactus
Copy link
Contributor Author

should also add J and K description to help (help_mode.c).

other than that looks good, thanks for doing this!

had a hard time coming up with a good description, let me know what you think. also added DEL.X and DEL.R help text.

@samdoshi
Copy link
Collaborator

note that make format never modified any of the files I changed. unsure if my formatting was sufficient enough, or if there was an issue with the command.

Don't worry about the formatting for now. make format has changed to only format the current commit. You need to run make format-all to format everything, but you'll probably find that formats more than you expect.

I've got a PR that'll drop soon with TravisCI format checking as well as updated guidance (and also addressing the clang-format versioning issues that have crept in... i.e. the current version formats some structs a bit better than older versions, but we need to get everyone on the same version).

@alpha-cactus
Copy link
Contributor Author

note that make format never modified any of the files I changed. unsure if my formatting was sufficient enough, or if there was an issue with the command.

Don't worry about the formatting for now. make format has changed to only format the current commit. You need to run make format-all to format everything, but you'll probably find that formats more than you expect.

I've got a PR that'll drop soon with TravisCI format checking as well as updated guidance (and also addressing the clang-format versioning issues that have crept in... i.e. the current version formats some structs a bit better than older versions, but we need to get everyone on the same version).

ok good to know I'll keep an eye out for updates. it did actually format my last change to help_mode.c, so I guess it's doing something.

@scanner-darkly
Copy link
Member

@tehn @samdoshi are we good to merge?

@samdoshi
Copy link
Collaborator

samdoshi commented Nov 2, 2018

are we good to merge?

If you're happy with it, I am. I haven't had much of a chance to look at it.

I take it that J and K are treated differently to I due to the way that I works with L? (This sentence feels very silly.)

Also @alpha-cactus, would you be okay with a squash-and-merge? (Or you could do some history rewriting yourself if you wanted to?)

@alpha-cactus
Copy link
Contributor Author

are we good to merge?

If you're happy with it, I am. I haven't had much of a chance to look at it.

I take it that J and K are treated differently to I due to the way that I works with L? (This sentence feels very silly.)

hmm not sure if I understand. do you mean when calling another script?

Also @alpha-cactus, would you be okay with a squash-and-merge? (Or you could do some history rewriting yourself if you wanted to?)

squash-and-merge - ya sure, is that something I would do? I'll look into it tonight if so.

@samdoshi
Copy link
Collaborator

samdoshi commented Nov 2, 2018

hmm not sure if I understand. do you mean when calling another script?

More that I lives in exec_vars_t (and therefore in the exec state), whereas J and K are in the scene state. My reading is that I needs special behaviour due to it being used with L.

squash-and-merge - ya sure, is that something I would do? I'll look into it tonight if so.

The squash and merge is done by the person that accepts the PR. What it will do, is turn all your commits into a single one. Alternatively you can 'rewrite history' with git and force push if you want to re-arrange to the commits to your own liking (but this is advanced git usage so don't feel you have to!).

@alpha-cactus
Copy link
Contributor Author

alpha-cactus commented Nov 2, 2018

More that I lives in exec_vars_t (and therefore in the exec state), whereas J and K are in the scene state. My reading is that I needs special behaviour due to it being used with L.

oh not sure about L, but the J and K implementation is more to do with the SCRIPT op. the idea behind J and K is that like I they can be used by a script without impacting the caller. the SCRIPT op passes I as part of the exec state, thusJ and K don't need to be passed as part of the exec state and live in the scene state. it's also helpful to have them in the scene state for the INIT ops and DELAY ops.

The squash and merge is done by the person that accepts the PR. What it will do, is turn all your commits into a single one. Alternatively you can 'rewrite history' with git and force push if you want to re-arrange to the commits to your own liking (but this is advanced git usage so don't feel you have to!).

okay feel free then. I'll have to look into the history thing, but it's not really necessary for this.

@alpha-cactus
Copy link
Contributor Author

actually maybe I misunderstood you there. if rewrite history would be preferable, then I'd be happy to do it for this merge.

@samdoshi
Copy link
Collaborator

samdoshi commented Nov 2, 2018

actually maybe I misunderstood you there. if rewrite history would be preferable, then I'd be happy to do it for this merge.

Nah, some people might prefer to do the history rewrite, so I thought I'd give you the opportunity, that's all I meant.

(the history rewriting stuff in git is pretty useful though, especially if you're using git for work)

@scanner-darkly I'll leave you to hit the merge button when you're ready.

@scanner-darkly scanner-darkly merged commit 25e14ee into monome:master Nov 3, 2018
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.

None yet

4 participants