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

quickfort script skeleton #159

Merged
merged 14 commits into from
Jul 28, 2020
Merged

Conversation

myk002
Copy link
Member

@myk002 myk002 commented Jul 19, 2020

DFHack/dfhack#499 High-level structure, help text, and input validation.

Each PR for this script should be release-ready in that functionality will have been tested and unimplemented features will display explanatory error messages instead of just erroring out.

I have PRs queued up for settings management, file listing, tokenization, and dig blueprint functionality. I'll submit PRs one a time so review feedback can be focused and so no two open PRs depend on one another (github doesn't really support a chain of PRs that build on one another -- the diffs will be against the target repo instead of the parent PR)

@myk002 myk002 changed the title WIP: Quickfort script skeleton (feedback requested) quickfort script skeleton Jul 19, 2020
quickfort.lua Outdated Show resolved Hide resolved
@myk002
Copy link
Member Author

myk002 commented Jul 25, 2020

travis docs check should pass after DFHack/dfhack#1612 is merged

@lethosor
Copy link
Member

lethosor commented Jul 25, 2020

Actually, the approach I adopted in ad67c79 is what scripts should use for their own modules now. Among other things, it respects script paths, while the mkmodule approach could result in the main script being loaded from another folder but the modules being loaded from hack/scripts/internal (I ran into this issue yesterday when updating advfort).

Not sure why the line numbers in the comments on 5177a12 are all wrong... I'll try to track that down. (FYI, we've switched from Travis to GitHub Actions, but the travis folder name is still used because several things depend on it.)

Update: for some reason, the run that triggers on 5177a12 is actually running on a merge of 1ec3d9b (one commit older) and ad67c79 (the current commit on master), according to https://github.com/DFHack/scripts/runs/910104637#step:4:375. I don't know why that's happening, but it's resulting in comments that aren't relevant to the changes you made.

Update 2: appears to be a GitHub actions bug: actions/checkout#299

@myk002
Copy link
Member Author

myk002 commented Jul 25, 2020

done. i'm liking reqscript better than require -- it doesn't need an explicit reload

@lethosor
Copy link
Member

Sure enough - I was about to comment that I didn't think it would handle reloading cleanly, but it turns out that there's a check for the file's modification time that I forgot about. Glad it works!

I'll merge the DFHack PR and see if that helps with the CI output here.

@myk002
Copy link
Member Author

myk002 commented Jul 25, 2020

thanks! test are passing now.

lethosor added a commit that referenced this pull request Jul 27, 2020
v2 uses the wrong commit sometimes (seen on #152 and #159 so far): actions/checkout#299
@myk002
Copy link
Member Author

myk002 commented Jul 28, 2020

Would it be possible to merge this PR? I'm worried about the PRs getting too far behind where I'm actually writing code -- suggested changes may end up causing merge conflicts.

@lethosor
Copy link
Member

My only hesitation is that I'm wrapping up 0.47.04-r2 (hopefully by around the end of this week, if all goes well), and I wouldn't want to include just this PR in a new release, since it looks like it'll mostly print "not implemented yet" messages. That's fine, and I definitely don't want to set a deadline for what you're doing, but I also don't want to confuse people by having this end up in a release in its current state. Some possibilities include:

  • We could merge this as-is, then deal with leaving it out of r2 later if the timing doesn't work out. Since some people read the docs, it would basically involve removing the script and changelog entry, then adding them back, which may end up creating merge conflicts with future changes.
  • We could merge this into a separate quickfort branch - I feel like I suggested this for another PR that I can't find now, but it might be a better fit for this PR, given that it depends more directly on future work that you have planned.
  • We could make a separate release branch (git-flow-style), although I don't think we've done that before in this repo, and it could get messy if a lot of other pre-release changes need to be merged in (there are some other PRs in this repo targeting r2).
  • We could wait for your next PR before merging this one. If you base it on this one (without rebasing), there shouldn't be merge conflicts, although it would be confusing to have an open PR be a subset of another one.

Thoughts? I know at least part of this complexity is due to our release process being messy; I'm fine with merging this and figuring out how best to handle it when a release rolls around (i.e. option 1) if you would prefer that.

@myk002
Copy link
Member Author

myk002 commented Jul 28, 2020

My concern is just around not having the PRs lag too far behind the code. I've got 500+ lines for dig and another ~500 for place, not to mention the code for file input and tokenization. I'm starting to worry that if I get significant comments on those PRs, I'll have to rework a lot of code. I'm not very concerned about getting the script into the current release (interested beta testers can always pull from my repo), so if that's the blocker, I'm totally fine with a quickfort branch.

@lethosor
Copy link
Member

Got it, I'll set up a quickfort branch. We can discuss whether the version that's there is release-ready when a release rolls around.

@lethosor lethosor changed the base branch from master to quickfort July 28, 2020 18:14
@lethosor lethosor merged commit c4133b1 into DFHack:quickfort Jul 28, 2020
@myk002 myk002 deleted the quickfort_skeleton branch July 28, 2020 21:42
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.

3 participants