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

Regenerate kst-based tests with current generator #108

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Mar 30, 2024

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
  • test generator changed slightly, so some constructs rendered in other way. I've updated them

@generalmimon
Copy link
Member

generalmimon commented Mar 30, 2024

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.

  • build-tests script contains error

In my opinion, build-tests should be removed. It was arbitrarily added by @sealmove in a direct push in the commit e7f899b without consulting anything and it is redundant (no offense, but this is what it looks like when push rights are granted generously - commit messages are mostly garbage, Nim has the most broken implementation in KS which inexplicably deviates from the rest of languages in various ways, to the point that I'm mostly avoiding it when working in the compiler and don't really want to touch it).

Using ./spec_kst_to_all directly gives you far more control - you can select multiple targets as for example -t java -t python, you can specify exactly which .kst specs you want to translate (instead of hardcoded --all-specs), you can ask the KST translator to generate directly to spec/{$LANG}/ folders via -f (which BTW build-tests completely ignores, needlessly resorting to mv "ks/out/$1"/* "$1/" + rm -rf ks/out), etc.

@Mingun
Copy link
Contributor Author

Mingun commented Mar 30, 2024

I suspected, that fixing generator to not insert newlines would be a better idea... Then blank lines commits could be removed, instead of fixing build-tests I could remove it (yes, I used the ./spec_kst_to_all --force in the end. Not the most understandable name, and underscores 😟 ...). But the other I think still valuable. It will save from unrelated changes when we will need to regenerate tests. In addition, some features were explicitly introduced into the generator, but were never used (compare floats using epsilon)

@generalmimon
Copy link
Member

generalmimon commented Mar 30, 2024

blank lines commits could be removed

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.

instead of fixing build-tests I could remove it

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.

(yes, I used the ./spec_kst_to_all --force in the end. Not the most understandable name, and underscores 😟 ...)

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 spec_*_to_* hackish scripts (spec_ruby_to_java, spec_python_to_perl, etc.) that I finally removed (except spec_kst_to_all) in f5973ff. So when these scripts existed, spec_kst_to_all kind of followed the pattern, but now we could indeed choose a better name. I'm open to suggestions - feel free to open a new PR/issue where you suggest a new name and we can discuss it.

I agree that the new name should use dashes - rather than underscores _ (I've never really thought about it), that way it will be consistent with other scripts.

Also, BTW, the -f/--force option could be enabled by default in my opinion (I'm a bit tired of always adding -f explicitly, but so far it hasn't bothered me that much to do anything about it), given that the generated test specs are tracked by Git and you can always rollback the changes you don't want, so you can't really lose any work if you use Git properly. Therefore, I really don't see the need for the special spec/ks/out output folder that is used by default.

@Mingun
Copy link
Contributor Author

Mingun commented Mar 31, 2024

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.

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).

I have no idea what's the point of making these gigantic PRs with a bunch of unrelated changes

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 ./spec_kst_to_all -t all --all-specs -f splitted in different commits for clarity and the goal is to decrease unrelated changes when you would be need to run this command purposely. So that is the reason why all those changes in one PR.

I'm open to suggestions - feel free to open a new PR/issue where you suggest a new name and we can discuss it.

I think, generate-kst-tests would be better name. I will see what I can do with it and open a new PR if that efforts will produce anything.

@Mingun
Copy link
Contributor Author

Mingun commented Apr 7, 2024

@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

generalmimon added a commit that referenced this pull request Sep 29, 2024
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)
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.

2 participants