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

Move copy to a plugin #165

Closed
jayvdb opened this issue Jan 16, 2019 · 28 comments
Closed

Move copy to a plugin #165

jayvdb opened this issue Jan 16, 2019 · 28 comments
Milestone

Comments

@jayvdb
Copy link
Member

jayvdb commented Jan 16, 2019

copy was a special key added to support non-templated files.

I believe it should be moved to be a plugin, and a copy entry could be included in the normal targets key like:

targets:
  - output: foo
    template: bar/foo
    template_type: copy

We can also make use of yaml datatypes to add more info to single row entries (also mentioned in the second half of #127 (comment)) like:

targets:
  - foo.py: foo.py.jj2  # implicit template type
  - !copy foo: bar/foo
  - !mako foo2: foo2  # explicit template type
@jayvdb
Copy link
Member Author

jayvdb commented Jan 16, 2019

Note I believe this should be a core plugin included in moban, as it is a really important feature for moban as a repository/project management tool.

The reasons to keep it separate from the main engine are that

  • the copy section is evaluated after the templating, whereas the order of processing sometimes needs a copy done before a template is invoked (if the template looks for a file exists). There should be one unified list of targets
  • the copy plugin currently has slightly different logic around it, which pops up unexpectedly e.g. All copy rules processed when CLI args request single target #166 . These bugs will be avoided if the copy is just another template type -- one which does zero substitutions.
  • it should be easy for people to create their own similar plugins if they need slightly different logic for copying.

@jayvdb jayvdb added this to the 0.4.0 milestone Jan 24, 2019
@jayvdb jayvdb pinned this issue Jan 24, 2019
@jayvdb jayvdb unpinned this issue Jan 24, 2019
@chfw
Copy link
Member

chfw commented Jan 25, 2019

what about template_type -> action?

action := 'template' | 'copy' | 'delete' | 'custom'

'custom' means if you supply your own action as a template, 'custom' will be enabled.

and for existing mobanfiles, default action is: 'template'.

@chfw
Copy link
Member

chfw commented Jan 26, 2019

Sounds good?

@jayvdb
Copy link
Member Author

jayvdb commented Jan 26, 2019

The naming of template_type to action is ok, but I dont see the benefit of adding template / custom.

I expected the internal copy plugin to be a template type / action just like jinja(2?), but the copy handler doesnt do much templating. It wouldnt implement many of the plugin hooks, because they are irrelevant.

One of the key reasons for moving copy to be an internal plugin, is that someone else should be able to create their own copy2 plugin with slightly different logic, if they needed to.

@jayvdb
Copy link
Member Author

jayvdb commented Jan 26, 2019

We might also deprecate the copy: yaml mapping.

I would switch all my moban usages to put the copy rules in targets: , which allows them to be processed in a more orderly manner, and I have some cases where I am currently looking for a target in targets: or in copy: to determine if the file is moban-managed or not, and it would be nice to stop doing that.

@chfw
Copy link
Member

chfw commented Jan 26, 2019

I am categorizing them in this way:

action -> template
action -> copy
action -> copy2
action -> delete
action -> any other actions

And under template, it can be jinja2, handlebars, slim, haml, mako..

we may expand the scope a bit further when there will be practical needs, for example:

  • pre-action: do something
  • post-action: do something

@chfw
Copy link
Member

chfw commented Jan 26, 2019

I particularly like -!copy a.output: a.template syntax. is there a yaml specification for it or we just treat it as a string?

@chfw
Copy link
Member

chfw commented Jan 27, 2019

a post-action can be written to cope with #38 , hence no long this ticket will require any action from moban but a custom action on the moban user. moban just need to enable the mechanism.

@chfw
Copy link
Member

chfw commented Jan 27, 2019

With .patch file, would there be a custom action to apply the patch to destination file?

@jayvdb
Copy link
Member Author

jayvdb commented Jan 28, 2019

There is value in having only one list of targets . Multiple keys, e.g. for copy: mean there are two sets of order, and only implicit order between those two sets.

I often want to do a copy of a template, and then use the template to generate another file. That is only possible if the targets are processed in order, and copy is done within targets.

If copy is done within targets, copy is just another template plugin.

@jayvdb
Copy link
Member Author

jayvdb commented Jan 28, 2019

pre-action and post-action feel like they will result in the same problems found in rpm and npm. Too many hooks, and someone always wants another hook.

One linear process is less problematic, even if it is slower. Efficiencies can be found by using make style optimisations in not regenerating targets unnecessarily (especially intermediate targets), and allowing the user to request only subsets of targets to be processed.

@jayvdb
Copy link
Member Author

jayvdb commented Jan 28, 2019

"!copy" is yaml schema datatypes, part of the normal yaml spec.
Most yaml libraries support it.

More info second half of #127 (comment)

@chfw
Copy link
Member

chfw commented Jan 28, 2019

I will take these conclusions:

  1. action as agreed, over template_type
  2. no need to go further to provide pre, post event handlers
  3. will look at the implementation of '!copy a.output:a.template'

@chfw
Copy link
Member

chfw commented Feb 2, 2019

one thing about the sequence: Under current implementation, there are no guarantee that: *.jj2 are processed earlier than *.handlebars. All jj2 files are processed sequentially as they appear in targets. When copy targets are mixed with templating targets, i.e.

targets:
   -!template 1.txt: 1.jj2
   -!copy 2.jj2: 2.source
   -!template 2.txt: 2.jj2

it is 'cheaper' to implement if either of one is true:

  1. their sequence dumbly assumed: all copies are executed before templated
  2. their sequence are not guaranteed.

@chfw
Copy link
Member

chfw commented Feb 2, 2019

I am sensing that this story will cause a period of in-stability of moban. The current architecture needs changing.

@chfw
Copy link
Member

chfw commented Feb 2, 2019

Hence, I am considering releasing what we have done as 0.3.10.

@chfw chfw mentioned this issue Feb 2, 2019
chfw added a commit that referenced this issue Feb 6, 2019
chfw added a commit that referenced this issue Feb 6, 2019
…newly installed packages are loaded, pypi-moban-pkg should be reloaded in the test, #165
@chfw
Copy link
Member

chfw commented Feb 7, 2019

@jayvdb , it seems that yaml tag syntax does not seem intuitive to non-developers.

Here is the actual file:

%TAG !copy! tag:yaml.org,2002:python/object:moban.definitions.
---
configuration:
  template_dir:
    - template-sources
targets:
  - !copy!CopyTarget
    source: simple.file.copy
    destination: file-in-template-sources-folder.txt

Here is the code to read it:

>>>from yaml import load
>>>with open('file_above.yaml', 'r') as f:
...    data=load(f)
>>>data['targets'][0]
<moban.definitions.CopyTarget object at 0x1015b7cf8>

And ruamel.yaml does not honor yaml 1.2

>>> from ruamel.yaml import YAML as y
>>> with open('.moban.yml', 'r') as d:
...     yd = y(typ='rt')
...     data = yd.load(d)
... 
>>> data['targets']
[ordereddict([('source', 'simple.file.copy'), ('destination', 'file-in-template-sources-folder.txt')])]

Do you have any more insights?

@chfw
Copy link
Member

chfw commented Feb 7, 2019

another alternative is that moban parse '!copy source: destination' itself. but then this is not yaml syntax

@chfw
Copy link
Member

chfw commented Feb 8, 2019

if moban does parse '!copy source:destination' itself, yaml module will intercept '!copy' before moban has it hand on it. So, this alternative route is a dead end.

targets:
  - foo.py: foo.py.jj2  # implicit template type
  - !copy foo: bar/foo
  - !mako foo2: foo2  # explicit template type

here is the proof

>>> from ruamel.yaml import YAML as y
>>> with open('test.yml', 'r') as f:
...     yaml = y(typ='rt')
...     data = yaml.load(f)
... 
>>> data
ordereddict([('targets', [ordereddict([('foo.py', 'foo.py.jj2')]), ordereddict([(<ruamel.yaml.comments.TaggedScalar object at 0x107316b00>, 'bar/foo')]), ordereddict([(<ruamel.yaml.comments.TaggedScalar object at 0x107316b70>, 'foo2')])])])
>>> data['targets'][1]
ordereddict([(<ruamel.yaml.comments.TaggedScalar object at 0x107316b00>, 'bar/foo')])
>>> for key, value in data['targets'][1].items():
...     print("key value: %s and style: %s" % (key.value, key.style))
...     print("the actual value %s " % value)
... 
key value: foo and style: None
the actual value bar/foo 

@chfw
Copy link
Member

chfw commented Feb 8, 2019

@jayvdb, what about the alternative syntax:

targets:
  - foo.py: foo.py.jj2  # implicit template type
  - a.txt: a.mako
  - b.txt: b.spitfire
  - copy:  # explicit copy action and grouping
    - foo: bar/foo
  - template-mako: # explicit template action using mako engine and grouping
     - foo2: foo2 
  - template-spitfire: # explicit template action using spitfire engine and grouping
     - bar3: bar3

@chfw
Copy link
Member

chfw commented Feb 8, 2019

waiting for further discussion.

@chfw
Copy link
Member

chfw commented Feb 8, 2019

if we go ahead with the following syntax:

%TAG !copy! tag:yaml.org,2002:python/object:moban.definitions.
---
configuration:
  template_dir:
    - template-sources
targets:
  - !copy!CopyTarget
    source: simple.file.copy
    destination: file-in-template-sources-folder.txt

Here are the pros and cons:

Positive: it simplifies moban code, because yaml parser gives me CopyTarget instances right a way. BUT this is only for yaml file. when moban reads .moban.json(or .moban.xls) in the future, this positive point is gone

Negative:

  1. we have to switch to PyYaml
  2. .moban.yml will be LOCKed to python due to TAGing to moban, but this may not be an issue as moban is python only.

@chfw
Copy link
Member

chfw commented Feb 9, 2019

After some serious thought, I would go for the following complete targets syntax:

targets:
  - foo.py: foo.py.jj2  # implicit template type
  - a.txt: a.mako
  - b.txt: b.spitfire
  - source: c.txt # complex copy syntax
     destination: c.txt
     action: copy
  - template: changelog.jj2 # explicit template type
     output: changelog.rst
     configuration: changelog.yml
     action: template
  - template: setup.py.jj2 # default action is template
     output: setup.py
     configuration: authors.yml
  - copy:  # explicit copy action and grouping
    - foo: bar/foo
    - foo2: bar/foo2
  - template-mako: # explicit template action using mako engine and grouping
     - foo2: foo2 
  - template-spitfire: # explicit template action using spitfire engine and grouping
     - bar3: bar3

The advantage are:

  1. no special use case comes from yml, hence it is easy to swap file format with json, etc.
  2. migrating from existing moban file becomes easy, just relocate existing copy section under targets and pad it with two spaces, add '- ' to copy, done!

@jayvdb , any comments before proceeding?

@jayvdb
Copy link
Member Author

jayvdb commented Feb 9, 2019

I disagree with "action: template" because it means copy isnt being implemented as a templaye type, but is given special status. The goal is to remove all architectual support that makes copy separate from other template types. Copy must be a template type, and its engine has no substitutions.

@chfw
Copy link
Member

chfw commented Feb 10, 2019

Yep, you were right I was giving 'copy' a special status, in parallel to 'template'. and template types are under 'template'.

However, I think I will accept your understand of 'copy', a special template engine, which does no substitutions and does simply content forwarding. Under this route, I would then agree with you on 'template_type' over my term 'action'.

Hence, here is the targets syntax of my understanding:

targets:
  - foo.py: foo.py.jj2  # implicit template type
  - a.txt: a.mako
  - b.txt: b.spitfire
  - template: c.txt # complex copy syntax
     output: c.txt
     template_type: copy
  - template: setup.py.unkown # explicit template type velocity for unknown.
     output: setup.py
     configuration: authors.yml
     template_type: velocity
  - copy:  # explicit copy action and grouping
    - foo: bar/foo
    - foo2: bar/foo2
  - template-mako: # explicit template action using mako engine and grouping
     - foo2: foo2 
  - template-spitfire: # explicit template action using spitfire engine and grouping
     - bar3: bar3

@chfw
Copy link
Member

chfw commented Feb 10, 2019

@jayvdb, what do you think about the new targets syntax?

@jayvdb
Copy link
Member Author

jayvdb commented Feb 10, 2019

Yup that looks great.

chfw added a commit that referenced this issue Feb 14, 2019
…#62 with current restriction on template type as key. What will happen if template type as group key is unknown, atm it will be treated as jinja2 template type
@chfw chfw mentioned this issue Feb 15, 2019
chfw added a commit that referenced this issue Feb 16, 2019
* ✨ initial prototype

* ✨ first working prototype that having relocated copy: into targets:. #165

* 📰 add missing unit test file

* ♿ deprecate copy: key word. #165

* 📰 add the missing file

* 🔨 code refactoring with TemplateTarget. replace all template tuples with TemplateTarget

* 💚 install pypi-mobans-pkg for testing

* 🔥 remove added test package: pypi-mobans-pkg

* 🐛 when new plugins are installed, call the lml scanner again so that newly installed packages are loaded, pypi-moban-pkg should be reloaded in the test, #165

* 🔨 refactor constants.py

* 🔥 remove previous fix for pypi-mobans-pkg not found. #165

* 📚 update copy action documentation and update integration tests 🔬

* 💄 minor update

* 🚜 relocating files

* 🚜 code refactoring on plugins.py

* 🚜 flatten concentrated files

* 🚜 fusing engines to template.py

* 🚜 relocate copier to plugins

* ✨ first working prototype: copy as template engine. #165

* 💚 fix unit test failure because of dependency

* 🔨 code refactoring, support template a directory as it used to, template-folder/a.jj2 become dest/a without jj2 suffix. it also means: copy-folder/me.copy becomes dest-folder/me but no effect if copy-folder/you, as it dest-folder/you

* 💚 try to fix broken unit tests in travis-ci

* 🐛 fix the bug where repo is not found if it is newly installed. #165

* 🔨 respect default template type

* 🔥 remove useless function as code evolves

* 🔨 code refactor magic copy label

* 🔬 test content forward engine, nickname copy engine. #165

* 🔥 remove useless report functions

* 🔥 remove unreachable test codes and remove unused code

* ✨ support group targets with template type, #165. And it is related to #62 with current restriction on template type as key. What will happen if template type as group key is unknown, atm it will be treated as jinja2 template type

* ✨ force template type regardless of individual template types. #165

* 💚 fix broken tests

* 🔬 more test coverage

* 🚜 rename docs directory

* 🔨 code refactoring

* 🔨 code refactoring on targets

* 🚜 re-organising file structures

* 🚜 re-organize repo related functionality

* 🚜 re-organize template target parsing functionality for moban file

* 📚 update documentation

* 📚 document 'copy' engine

* 🔨 black all source code

* 📚 update change log

* 💄 update gitignore

* 🔬 squeeze more test coverage from test code

* :micrscope: more test coverage

* 🔬 more tests on mobanfile targets

* 🔨 relocate test files

* 🔬 more tests on mobanfile targets

* 🚜 relocate test file

* 🔬 more tests

* 📚 update documentation index

* 📚 update change log
@chfw chfw closed this as completed Feb 16, 2019
@chfw
Copy link
Member

chfw commented May 22, 2020

A new syntax was invented in #382 : delete!.

Originally, '!delete' was proposed but was found no feasible. However, 'delete!' is deemed by yaml as pure string.

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

No branches or pull requests

2 participants