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

Some fixes for the release blog post #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KOLANICH
Copy link

@KOLANICH KOLANICH commented Oct 18, 2020

No description provided.

@KOLANICH KOLANICH force-pushed the 0.9_msg_fixes branch 2 times, most recently from 7df0afb to 3108987 Compare October 18, 2020 17:03
several handy features (like `valid`ations and little-endian "bit-sized
types"), fixes a lot of bugs and includes quite a few infrastructure
Copy link
Member

@generalmimon generalmimon Oct 18, 2020

Choose a reason for hiding this comment

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

I would keep the word integers, because type is too broad in my opinion:

Suggested change
several handy features (like `valid`ations and little-endian "bit-sized
types"), fixes a lot of bugs and includes quite a few infrastructure
several handy features (like `valid`ations and little-endian "bit-sized
integers"), fixes a lot of bugs and includes quite a few infrastructure

What was your intention when changing it to types?

Copy link
Author

Choose a reason for hiding this comment

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

Because endianness issue is not about bit-sized integers alone, it is about both bit- and byte-sized integers within types which fields (which can have non-integer type) can occupy non-integer count of bytes. Endeanness is here because of remapping. The common way to deal with bit-sized stuff is using bitwise logical operations, since it is the only mean of doing that on most of CPUs (maybe except some Intel ones with integrated FPGAs), and it is proficient to use CPU-native remapping, little-endian (no designers of modern systems are mad enough to use BE as native byte order). Such remapping usually deals with chunks longer than bytes, and it is position of numbers and structs relative to the chunks boundaries defines the way pieces of chunks are cut, reordered and joined. So, IMHO, endianness for bit-sized integers alone makes no sense as it is at all (how to cut b9le, 0b11111111_1, , 0b1_11111111, or maybe 0b11111_1111?), what makes sense is the endianness of their container that has the bit-size of multiple of chunk length and aligned to chunks boundaries and where the field is within the container.

@@ -63,7 +65,7 @@ fixes a lot of bugs and includes quite a few infrastructure improvements.
* C++: add C++11 mode
* Add `--cpp-standard` CLI option: pass `--cpp-standard 11` to enable C++11 mode (`98` is default)
* C++11 target:
* uses `#pragma once` (instead of `#ifndef FOO_H_` header guards)
* uses `#pragma once` (instead of `#ifndef FOO_H_` header guards) ([25fb1eee61d07bdc8b199445776836ce9a6606ef](https://github.com/kaitai-io/kaitai_struct_compiler/commit/25fb1eee61d07bdc8b199445776836ce9a6606ef))
Copy link
Member

@generalmimon generalmimon Oct 18, 2020

Choose a reason for hiding this comment

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

I'm pretty sure that 40 characters of random rubbish will only distract and disturb the user, and most likely no one will actually ever open that commit - it's just absolutely pointless for the expected audience.

To be frank, I was already quite sceptical about the issue references, because the ubiquitous (#1234) brackets have also potentially a disturbing effect. The only reason why I left them somewhere is that there is usually some explanation of how to use the particular feature, so they serve as a temporary replacement for a proper documentation. But you can see that whenever there is the documentation available (e.g. the little-endian bit ints), I use (docs) instead of (#155).

Copy link
Author

@KOLANICH KOLANICH Oct 18, 2020

Choose a reason for hiding this comment

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

Some of these changes were a big surprise for me. I mean I am subscribed to the bug trackers, and I was really surprised to find these changes, since I haven't remembered them. So I searched the bug tracker, and found nothing. So I had to dig the commit history. So, the links to commits convey the following information sparing the users from digging the history themselves:

  1. The addition of the feature was not visible in the issue tracker
  2. when, where and by whom it was introduced.
  3. what exactly effect it does and how to use it.

Copy link
Member

@generalmimon generalmimon Oct 28, 2020

Choose a reason for hiding this comment

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

So, the links to commits convey the following information sparing the users from digging the history themselves

Why would normal users dig the history in the first place? What would be the reason?

  1. The addition of the feature was not visible in the issue tracker

IMHO it makes sense to open an issue only if there's an intent to give people know that the particular bug fix or feature is being implemented, or if there is anything to discuss. If it's something small and obvious, creating an issue is redundant, and it would often take longer than the code implementation itself.

  1. when, where and by whom it was introduced.

This is exactly what commands git annotate and git blame are for. Finding the commit where #pragma once was introduced is actually even easier using the GitHub web GUI:

  1. Search GitHub for org:kaitai-io pragma once and click the line number containing usePragmaOnce in the compiler source code (line 17 on the screenshot):

    image

  2. Click the meatballs menu (3 horizontal dots) to the left from the line number and choose View git blame:

    image

  3. Click the commit link on the left to go to the the commit detail page:

    image

  1. what exactly effect it does and how to use it.

Yeah, this is a point we can agree on, that's definitely worth knowing for the user. This is what I meant with referencing issues as the temporary replacement for documentation in #17 (comment). But l don't get how linking to a commit with the source code helps here: do you really think that a typical user will go through the source code to understand how he's supposed to use the feature? I don't think so.

Moreover, just in context of the #pragma once , this argument doesn't make much sense - it's too simple and obvious to deserve any detailed docs. And one doesn't use it directly, it's more of a generated code improvement, the user actually doesn't need to know about it. And if one doesn't understand what pragma once means, all they need to do is to search it with Google.

Given it's that much obvious and simple, it also undermines the need of knowing who, when and where introduced it.

Copy link
Author

Choose a reason for hiding this comment

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

Finding the commit where #pragma once was introduced is actually even easier using the GitHub web GUI:

This was what I did (and for other features too) :) But even if we use GH for it, it is digging git history, just GH-assisted.

Why would normal users dig the history in the first place? What would be the reason?

Just to know the feature enough. Not only the fact that it exists, but also what exactly it does and what is it intended to be used for.

do you really think that a typical user will go through the source code to understand how he's supposed to use the feature?

Yes. Digging into source code is not the best way, but if it is the only way (since there is no docs for some features), one has to. If one is determined to get known how to use the feature, lack of docs won't stop him.

Moreover, just in context of the #pragma once , this argument doesn't make much sense - it's too simple and obvious to deserve any detailed docs.

It doesn't deserve the detailed docs, but a user seeing it deserves the clarification for the question "WTF? I am monitoring the repo, but I don't remember this feature. Have I messed anything?" So, a link to a commit is an answer "No, you haven't missed anything, it was just silently added to the repo".

Copy link
Member

@generalmimon generalmimon Oct 28, 2020

Choose a reason for hiding this comment

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

"No, you haven't missed anything, it was just silently added to the repo".

Fortunately, Git doesn't allow adding anything silently, all changes are done via commits. If you really want to be notified of every commit and comment that happened in the kaitai-io org, use the GitHub API for public organization events: https://api.github.com/orgs/kaitai-io/events

I don't really understand why GitHub doesn't allow accessing the activity feed of an arbitrary org of which you're not a member directly in the web GUI or RSS format, but looking at the API response I link above, there is really all activity I see in my kaitai-io dashboard :-)

So if you're determined to never miss a thing anymore, even the small and insignificant ones like #pragma once, give it a few hours and I believe that you can handcraft a Python script that generates an RSS feed or a fancy nice HTML+CSS human-friendly format or whatever output you'd like to. There are also numerous libraries to simplify the work with the GitHub API like https://github.com/PyGithub/PyGithub.

Just note that GitHub API has a rate limit of 60 requests per hour for unauthenticated and 5000 per hour for authenticated requests, which is enough though for almost any use. See https://developer.github.com/v3/#rate-limiting.


And my decision on the useless kaitai-io/kaitai_struct_compiler@25fb1ee commit ref hasn't changed, there is no place for it in the release highlights.

@KOLANICH KOLANICH force-pushed the 0.9_msg_fixes branch 3 times, most recently from 0249688 to 2fdc12c Compare October 18, 2020 22:04
@@ -99,12 +101,19 @@ fixes a lot of bugs and includes quite a few infrastructure improvements.
Gradle plugin](https://github.com/valery1707/kaitai-gradle-plugin)
* Infrastructure updates:
* Unstable binary builds are available for all platforms after every CI build at Bintray ([#63](https://github.com/kaitai-io/kaitai_struct/issues/63))
* KSY language reference replaced with [documentation](https://doc.kaitai.io/ksy_diagram.html) generated from JSON schema
* KS language reference replaced with [documentation](https://doc.kaitai.io/ksy_diagram.html) generated from [the JSONSchema](https://github.com/kaitai-io/ksy_schema)
Copy link
Member

Choose a reason for hiding this comment

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

It's called JSON Schema, not JSONSchema, JSONS chema or whatever:

Suggested change
* KS language reference replaced with [documentation](https://doc.kaitai.io/ksy_diagram.html) generated from [the JSONSchema](https://github.com/kaitai-io/ksy_schema)
* KS language reference replaced with [documentation](https://doc.kaitai.io/ksy_diagram.html) generated from [the JSON Schema](https://github.com/kaitai-io/ksy_schema)

* Implement little-endian bit-sized integers ([docs](https://doc.kaitai.io/user_guide.html#bit-ints-le))
* Support choosing endianness using `le` / `be` suffix: `type: b12le`, `type: b1be`
* Add `meta/bit-endian` key for selecting default bit endianness (`le` / `be`)
* Implement little-endian-based bit-sized types ([docs](https://doc.kaitai.io/user_guide.html#bit-ints-le))
Copy link
Member

@generalmimon generalmimon Oct 28, 2020

Choose a reason for hiding this comment

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

What about just using the established terminology from the User Guide (https://doc.kaitai.io/user_guide.html#bit-ints-le) to which you had no serious objections (just "some offtop")?

Suggested change
* Implement little-endian-based bit-sized types ([docs](https://doc.kaitai.io/user_guide.html#bit-ints-le))
* Implement bit-sized integers in little-endian order ([docs](https://doc.kaitai.io/user_guide.html#bit-ints-le))

It would be also OK to use little-endian layout for example.

Yeah, I'm still not a fan of bit-sized types, and I don't really understand how #17 (comment) justifies it.

Copy link
Author

Choose a reason for hiding this comment

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

The problem is that that feature is mainly not about integers in little-endian order. Concept of endianness makes sense only for the stuff that have the size of multiple of endianness granularity (8 bit currently) and is aligned with that granularity. And it is not restricted to integers, it is just that bits sequences can be processed as integers and in order to process them on CPUs they have to be treated this way.

@@ -72,6 +73,7 @@ fixes a lot of bugs and includes quite a few infrastructure improvements.
* Runtime API changes:
* Add exceptions `Validation{Not{Equal,AnyOf},{Less,Greater}Than,Expr}Error` inheriting from common ancestor `ValidationFailedError` - thrown on failed validations defined with `valid` or `contents` key ([#435](https://github.com/kaitai-io/kaitai_struct/issues/435))
* Add method `read_bits_int_le` for parsing little-endian bit-sized integers ([docs](https://doc.kaitai.io/user_guide.html#bit-ints-le))
* Allow third-party `process`ors to be used ([#457](https://github.com/kaitai-io/kaitai_struct/issues/457)).
Copy link
Member

@generalmimon generalmimon Oct 28, 2020

Choose a reason for hiding this comment

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

kaitai-io/kaitai_struct#457 is only some weird Q&A style discussion, I'd rather mark it with the blue label question. @GreyCat only mentions the creation of the https://github.com/kaitai-io/kaitai_compress repo, but there is no mention of any changes in KSC or runtime libraries.

I think this is actually a made-up point: the kaitai_compress repo apparently didn't require any such changes. See git blame of file kaitai_struct_compiler / shared/.../JavaCompiler.scala:

image

The commit message states that the last change to that line was done when adding custom processing routines (https://doc.kaitai.io/user_guide.html#custom-process) and it dates back to 2017-07-21. So it's not a change introduced in 0.9, because 0.8 was released on 2018-02-05.

So please remove it:

Suggested change
* Allow third-party `process`ors to be used ([#457](https://github.com/kaitai-io/kaitai_struct/issues/457)).

Comment on lines +111 to +112
* Added an [official library of compression algorithms](https://github.com/kaitai-io/kaitai_compress) to be used in `process`.
* Created [a list of ![awesome](https://camo.githubusercontent.com/1997c7e760b163a61aba3a2c98f21be8c524be29/68747470733a2f2f617765736f6d652e72652f62616467652e737667) projects related to Kaitai Struct](https://github.com/kaitai-io/awesome-kaitai)
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the imperative style (add!, support!, allow!, implement!, ...) of the release notes.

Suggested change
* Added an [official library of compression algorithms](https://github.com/kaitai-io/kaitai_compress) to be used in `process`.
* Created [a list of ![awesome](https://camo.githubusercontent.com/1997c7e760b163a61aba3a2c98f21be8c524be29/68747470733a2f2f617765736f6d652e72652f62616467652e737667) projects related to Kaitai Struct](https://github.com/kaitai-io/awesome-kaitai)
* Add an [official library of compression algorithms](https://github.com/kaitai-io/kaitai_compress) to be used in `process`.
* Create [a list of ![awesome](https://camo.githubusercontent.com/1997c7e760b163a61aba3a2c98f21be8c524be29/68747470733a2f2f617765736f6d652e72652f62616467652e737667) projects related to Kaitai Struct](https://github.com/kaitai-io/awesome-kaitai)

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