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

Support and use YAML blackboxes #2037

Merged
merged 7 commits into from
Feb 3, 2022
Merged

Support and use YAML blackboxes #2037

merged 7 commits into from
Feb 3, 2022

Conversation

martijnbastiaan
Copy link
Member

@martijnbastiaan martijnbastiaan commented Jan 17, 2022

Added support for YAML blackboxes. Clash will now pickup on files with a
.primitives.yaml extension. While we recommend upgrading your primitive files
to the new format, old style primitives are still supported.

Depending on whether you use Stack or Cabal you can execute the following to
upgrade your primitive files:

cabal run clash-lib:v16-upgrade-primitives -- --help

Stack users can use:

stack exec --package clash-lib v16-upgrade-primitives -- --help

Both commands assume you're running them in an up-to-date
(starter) project environment.

@martijnbastiaan martijnbastiaan mentioned this pull request Jan 17, 2022
2 tasks
@alex-mckenna alex-mckenna added this to the 1.6 milestone Jan 19, 2022
@martijnbastiaan martijnbastiaan changed the title Add 'v16-upgrade-primitives' to clash-lib Support and use YAML blackboxes Jan 19, 2022
@alex-mckenna
Copy link
Contributor

+14,491 −16,124

Someone's gunning for top contributor 2022

@martijnbastiaan
Copy link
Member Author

I wish there was a [no-count-diff] :P

@leonschoorl
Copy link
Member

This maybe hard or impossible with the high-level YAML api.
But I feel it would be make the generated .primitives.yaml much nicer to read if we could get the conversion tool to put the type field, before the template field.

@alex-mckenna
Copy link
Contributor

I doubt easy. Objects in aeson are just stored as maps then traversed over. I think you're at the mercy of how the map is ordered internally

@martijnbastiaan
Copy link
Member Author

Yeah.. that'd be the Ord instance of Text :(

@leonschoorl
Copy link
Member

You're adding the Clash_CoSim_CoSimInstances.primitives.yaml, but the corresponding Clash_CoSim_CoSimInstances.primitives was never in git.
It's generated and .gitignored
I think we need to do the same here and update the generation code.

@martijnbastiaan
Copy link
Member Author

But I feel it would be make the generated .primitives.yaml much nicer to read if we could get the conversion tool to put the type field, before the template field.

@leonschoorl Python's dicts are insertion ordered, so I've abused that to generate YAML files with the pretty order:

#!/usr/bin/env python3
import yaml
import sys

# https://github.com/yaml/pyyaml/issues/240#issuecomment-1018712495
def str_presenter(dumper, data):
    if len(data.splitlines()) > 1:  # check for multiline string
        return dumper.represent_scalar('tag:yaml.org,2002:str', data, style='|')
    return dumper.represent_scalar('tag:yaml.org,2002:str', data)

yaml.add_representer(str, str_presenter)
yaml.representer.SafeRepresenter.add_representer(str, str_presenter) # to use with safe_dum

def reorder_dicts(o, order=("name", "kind", "type", "template")):
  if isinstance(o, list):
    return list(map(reorder_dicts, o))
  if isinstance(o, dict):
    # Reorder dict
    new_dict = {}
    for s in order:
      if s in o:
        new_dict[s] = o.pop(s)
    new_dict.update(o)

    # Traverse all values in dict and reorder _them_
    reorderd = {k: reorder_dicts(v) for k, v in new_dict.items()}
    return reorderd
  if isinstance(o, (str, int, bool)):
    return o
  else:
    raise TypeError(f"Unexpected type: {type(o)}")

def reorder_dicts_in_file(path):
  with open(path) as fp:
    prims = yaml.safe_load(fp)

  prims = reorder_dicts(prims)
  prims = yaml.dump(prims, sort_keys=False, line_break="\n")
  with open(path, "w") as fp:
    fp.write(prims)

if __name__ == '__main__':
  import glob

  for path in glob.glob(sys.argv[1], recursive=True):
    reorder_dicts_in_file(path)

Aeson should probably use insertion ordered maps too..

@martijnbastiaan
Copy link
Member Author

I think we need to do the same here and update the generation code.

Done

Copy link
Member

@leonschoorl leonschoorl left a comment

Choose a reason for hiding this comment

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

I think we still need:

  • documentation
    • changelog
    • some documentation on (how to run) the upgrade tool
    • the examples in Clash.Tutorial still use the old format
  • maybe a test that uses the old format, so we don't accidentally break it

@martijnbastiaan
Copy link
Member Author

@leonschoorl All done.

Copy link
Contributor

@alex-mckenna alex-mckenna left a comment

Choose a reason for hiding this comment

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

Yeah, seems fine. I didn't spot any, but since all the primitives are being changed it might be worth trying to see if any have obviously wrong type comments, i.e. using Undefined instead of NFDataX

template-haskell >= 2.0.0,
Cabal
text,
yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Bounds for yaml since it's not a boot library

clash-lib/clash-lib.cabal Outdated Show resolved Hide resolved
clash-lib/clash-lib.cabal Show resolved Hide resolved
Comment on lines +69 to +76
[ ("name", "aaaa_really_should_be_name_but_renamed_to_get_the_sorting_we_like")
, ("type", "really_should_be_type_but_renamed_to_get_the_sorting_we_like")
Copy link
Contributor

Choose a reason for hiding this comment

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

A masterpiece

@alex-mckenna
Copy link
Contributor

+16,061 −17,015

It just gets chonkier 👀

@martijnbastiaan
Copy link
Member Author

:(

@alex-mckenna
Copy link
Contributor

As for me, I like big diffs and I cannot lie. -- Martijn Bastiaan

martijnbastiaan and others added 7 commits February 3, 2022 18:15
Files ending in `.primitives.yaml` are now interpreted as YAML blackbox
files.

Fixes #2038
This is in preparation of converting all primitives to YAML. Doing a
rename first preserves a usable history. If we don't do this, Git won't
see a relation between the old and new primitive files.
This makes sure `name` is at the top and and `type` is just above
`template`.
Their keys are stored in HashMaps whose order we can't predict
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