zoranbosnjak / asterix-specs

EUROCONTROL asterix specifications in structured format.
https://zoranbosnjak.github.io/asterix-specs/
BSD 3-Clause "New" or "Revised" License
23 stars 14 forks source link

Modification of AST file for CAT062 #5

Closed dsalantic closed 3 years ago

dsalantic commented 3 years ago

Here are my first (but not final) modifications for CAT062. Mainly for improvement of capital letter consistency, but also some additions. Please review them and we can discuss each change in comments.

zoranbosnjak commented 3 years ago

Most changes are OK. I appreciate the effort to make it consistent. Here are some remarks to discuss:

  1. Can you make the consistency rules you are following explicit, so that I can convert them to validation rules, to spot the problems automatically. For example, some rules we can actually check:

    • Each item's short name (like "SIC") is at most N characters long (e.g. 15).
    • Short name shall only contain ascii subset of characters, between ? and ?, and shall not contain [space, "/", "\n", "\t"...]
    • First character of the short name must be capital or number ("010" is short name too). Maybe all capital?
    • Titles shall be capitalized, except words [in, of, ...], like "System Identification Code".
    • Table entries shall be capitalized, like "Default value", not "default value".
    • ... and other rules like these?
  2. The "definition" field is not like "title". The definition (in general) is a normal sencence or more sentences, so it "Shall Not Be Capitalized Like This" (I am refering to the change in I062/060).

  3. Some search and replace on the word "default" slipped into the notes, likely by mistake.

  4. What do you think about the idea to have all short names "CAPITAL" only? This means "LAT" instead of "Lat"? I have just run converter ... --list (get item listing), to observe that most short names are CAPITAL already. This rule could be easily checked and enforced by the validator. I think this will pay off on the long run (one consistency problem less).

  5. I would suggest to avoid repetitions in item accessor like "500/APW/APWLAT" and to write "500/APW/LAT" instead (or "500/APW/Lat" if you don't like the idea from the previous item). There are some more names like this to be fixed. Just let me know what do you think about general guideline to follow for such names.

  6. Do we allow short name like ORIENTATION, LENGTH, HEADING... due to the size. I would say yes.

  7. Just spotted an unrelated old mistake on item 500/ATW. It shall be "ATV". Please fix it while you are on the file.

  8. It looks like I062/380/ACS structure description comes from another document. Is it a good idea to disect this item inside ast?

dsalantic commented 3 years ago
  1. I agree with your rules. Additionally:

    • Short name only with : A-Z, 0-9
    • Table entries with first letter capitalized, other can be as it is in specification document (some capital, some not) but without ending dot
  2. OK, fixed

  3. OK, fixed

  4. Agreed, fixed

  5. Agree

  6. OK, but let's put some limitation (e.g. 15)

  7. OK, fixed

  8. Agree. It is actually a BDS registar so I will move it to BDS specification.

See my new changes in commit.

zoranbosnjak commented 3 years ago

Great! Is this a final commit on this file? In any case, before merging to master, it would be nice to cleanup all files in this spirit. I have created a new branch named "consistency" for this purpose. Please pull the latest update, from command line:

git pull https://github.com/zoranbosnjak/asterix-specs.git consistency

...or otherwise make sure to continue from 53d9379. I will adjust the validator, according to the discussion, so that we can run it against all files. I will need some time to implement it. If you would like to update other files in the meantime, please do so. In particular, if there are more rules that can be automatically checked, let me know.

zoranbosnjak commented 3 years ago

Please check revision 9f38ddc. The validation is now extended as discussed (except 5. - need some more time). There are 2 levels of validation problems (warnings and errors). I have put item short name rules under more strict validations (errors) and the rest under warnings (such that it won't break the build, but you can still see them if you explicitely turn them on).

Please have a look and let me know if the validation works as expected. To run the validation, you will need to rebuild the converter and run it with "--validate" or "--validate --warnings".

To avoid concurrent editing of the same files, I will wait for you (@dsalantic) to finish whatever you wish to modify on the definitions. Let me know when done and I will take it from there. Thanks a lot.

dsalantic commented 3 years ago

I fixed validation error in cat062 1.18, but there are too many warnings because you apply capital letter formatting to table elements. Can you remove table elements validation, I don't think we need to capitalize them, just the element names.

zoranbosnjak commented 3 years ago

Sure, I can remove table lines checking. I assume we still want to check the first word to start with capital letter, but not each word, correct?

dsalantic commented 3 years ago

Correct. Just first word.

zoranbosnjak commented 3 years ago

Fixed in revision 03c3fc0.

dsalantic commented 3 years ago

OK, I pushed CAT062 version without syntax warnings. I believe this is now ready. There are many changes to original version, I hope we were no too strict with all the rules.