-
Notifications
You must be signed in to change notification settings - Fork 31
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
Regenerate kst-based tests with current generator #108
base: master
Are you sure you want to change the base?
Conversation
To be honest, I don't see much value in most of these changes. As far as I'm concerned, this PR is more of a candidate for closing than merging. In particular, the commits adding a blank line are useless, they're just noise. I think it would be better to fix the spec generators to not generate the blank line.
In my opinion, Using |
I suspected, that fixing generator to not insert newlines would be a better idea... Then blank lines commits could be removed, instead of fixing |
Yes, please do a force push with the blank lines commits dropped. The mere presence of these noisy changes makes it difficult to see anything else.
Rather just don't touch it in this PR. I have no idea what's the point of making these gigantic PRs with a bunch of unrelated changes, you're just increasing the chance that all of your work (some good, some less so) in the bulk PR will go stale. You can open multiple pull requests at once. Small and compact PRs are easier to review and discuss.
AFAIK, the name has remained that way historically - before the introduction of the KST translator (kaitai-io/kaitai_struct#253), a new test spec was first written in one language (mostly Ruby) and there used to be an entire family of I agree that the new name should use dashes Also, BTW, the |
That is why these changes was splitted to dedicated commits. You can review commit-by-commit, some commits even suggest to enable whitespace ignored mode to see the real difference (not in this PR, however).
If you were to agree that you need to add blank lines, then this work would have to be done one way or another. I didn't quite understand what kind of changes are unrelated? All changes are the result of a single command
I think, |
@generalmimon, I left here only changes that is related to explicitly made changes in generator / expression translator. All other changes will be suggested in other PRs. Please look again |
In some target languages like JavaScript or Python, this newline looks awkward and doesn't make sense, so I think it's best to remove it (finally!). This has already been discussed, see #108 (comment)
While working on another task, I have to regenerate tests and noticed, that:
build-tests
script contains error because of that tests never regenerated because their was generated in one temporary folder, and script tried to move other(not existent) folder and in the end temporary folder is deleted