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

Add mode option for file creation #271

Closed
wants to merge 1 commit into from

Conversation

dibby-dibby
Copy link

Add mode option for file creation

@FlorianWilhelm
Copy link
Member

@dibby-dibby, thanks for this PR but couldn't you achieve the same thing using the command umask before you call putup?

@dibby-dibby
Copy link
Author

Sorry for the late response, but I added it to be able to apply specific mode for each file I add in my extension's "define".

for example, I want to add 2 bash scripts to the project, and I want them to be already created with execution permission.

@FlorianWilhelm
Copy link
Member

Ok, got it, thanks. Could you fix the unit tests and add new ones that test your PR? Ignore the Python 3.5 test for now.

@abravalheri
Copy link
Collaborator

Guys, if we are changing the API, would it be possible to document it in https://github.com/pyscaffold/pyscaffold/blob/master/docs/extensions.rst ?

@FlorianWilhelm
Copy link
Member

Good point. It should be documented also we could use this chance to rethink the code a bit. Things like
content, mode = content
are really not that nice to read.

@abravalheri
Copy link
Collaborator

abravalheri commented Jul 13, 2020

Good point. It should be documented also we could use this chance to rethink the code a bit. Things like
content, mode = content
are really not that nice to read.

So, I think the problem here is that the original API I proposed some time ago is starting to present a genuine symptom of Primitive Obsession with those flags... They are deep intricate dependencies of the core, while a more flexible pattern would be inverting the conditional checks and use some king of strategy pattern.

Indeed to choice of primitive data structures (dicts, tuples, int/string flags) was deliberate: I wanted to have something not disruptive from the original implementation in PyScaffold v2, and that was easy to explain to a newcomer.
This code smell is present is all the extension mechanism for the same reasons. We have a plain dictionary instead of a custom class for the project structure, a plain list for the actions instead of a custom class, and so on... The way I compensate for that is with the the helpers module, and by keeping the design very close to a functional programming style.

While we could implement a File object as the leaf of a Struct object tree and wrap everything in a custom object, sincerely, I would like to keep things simple, because the current API it fulfil its purposes.

However I could implement a compromise: I mentioned that a strategy pattern could replace the flag. If we consider the single responsibility principle, a strategy pattern can be basically reduced to a callable object. And plain old functions are callable objects. So instead of a flag we could accept a function.

In the best spirit of mypy (if recursive types were allowed):

from pathlib import Path
from typing import Callable, Dict, Protocol, Text, Tuple, Union

PyScaffoldOpts = dict
FileContent = Union[None, Text]
FileOperation = Callable[[Path, FileContent, PyScaffoldOpts], None]
# ^  The `Stategy` callable.
#    It does not return anything but can create a file as side-effect
FileWithOperation = Tuple[FileContent, FileOperation]
FileNode = Union[FileContent, FileWithOperation]
DirTree = Dict[str, Union[FileNode, "DirTree"]]
ProjectStructure = DirTree

Particularly, I would implement NO_OVERWRITE or NO_CREATE as FileOperation "factories" (high order functions/class/wrappers/decorators: Callable[[FileOperation], FileOperation], and a default Create, file operation, so we can compose them together...

class Create:
   ...
    def __call__(path: Path, content: FileContent, _opts: dict):
        if content:
            path.write_text(content)
   
class NoOverwrite:
    def __init__(self, wrapped: Optional[FileOperation] = None):
        self.wrapped = wrapped or Create()

    def __call__(path: Path, content: FileContent, opts: dict):
        if opts["force"] or not path.exists():
            self.wrapped(path, content, opts)

class SkipOnUpdate:  # the NO_CREATE name is confusing because it does create something...
    def __init__(self, wrapped: Optional[FileOperation] = None):
        self.wrapped = wrapped or Create()

    def __call__(path: Path, content: FileContent, opts: dict):
        if opts["force"] or not opts["update"]:
            self.wrapped(path, content, opts)

# All the classes here could be functions/factories but I like the aesthetics of CamelCase 😝. Alternatively:

def create(path, content, _opts): ...

def no_overwrite(wrapped: FileOperation = create):
    def __call__(path: Path, content: FileContent, opts: dict):
        if opts["force"] or not path.exists():
            wrapped(path, content, opts)
    return __call__


def skip_on_update(wrapped: FileOperation = create):
    def __call__(path: Path, content: FileContent, opts: dict):
        if opts["force"] or not opts["update"]:
            wrapped(path, content, opts)
    return __call__

This changes would simplify the struct module, and would even make the pyscaffold.update:apply_update_rules action unnecessary. But I also believe that the helpers.modify should change:

# INSTEAD OF
def modify(
    struct: ProjectStructure,
    path: Path,
    modifier: Callable[[FileContents], FileContents],
    update_rule: Literal[None, NO_CREATE, NO_UPDATE]): ...

# WOULD BE
def modify(
    struct: ProjectStructure,
    path: Path,
    modifier: Callable[[FileContents, FileOperation], FileWithOperation]): ...
    # The `update_rule` would be absorbed by the modifier callback, which would allow using the factories directly

abravalheri added a commit to abravalheri/pyscaffold that referenced this pull request Jul 18, 2020
This is an redesign attempt to solve the limitations explained in pyscaffold#271.
Currently the API is suffering from primitive obsession (a part of it was a
deliberate design choice to make the extension mechanism easily
understandable even for beginners).

The changes introduced here improve a bit, making the design more flexible
by replacing `FileOp` integer flags with functions.
Most of the primitive obsession is kept due to the same design decisions that
motivated the first implementation.

The following commits should ensure no errors when running PyScaffold and
add documentation.

Closes pyscaffold#271.
abravalheri added a commit to abravalheri/pyscaffold that referenced this pull request Jul 19, 2020
This is an redesign attempt to solve the limitations explained in pyscaffold#271.
Currently the API is suffering from primitive obsession (a part of it was a
deliberate design choice to make the extension mechanism easily
understandable even for beginners).

The changes introduced here improve a bit, making the design more flexible
by replacing `FileOp` integer flags with functions.
Most of the primitive obsession is kept due to the same design decisions that
motivated the first implementation.

The following commits should ensure no errors when running PyScaffold and
add documentation.

Closes pyscaffold#271.
abravalheri added a commit to abravalheri/pyscaffold that referenced this pull request Jul 19, 2020
This is an redesign attempt to solve the limitations explained in pyscaffold#271.
Currently the API is suffering from primitive obsession (a part of it was a
deliberate design choice to make the extension mechanism easily
understandable even for beginners).

The changes introduced here improve a bit, making the design more flexible
by replacing `FileOp` integer flags with functions.
Most of the primitive obsession is kept due to the same design decisions that
motivated the first implementation.

The following commits should ensure no errors when running PyScaffold and
add documentation.

Closes pyscaffold#271.
abravalheri added a commit to abravalheri/pyscaffold that referenced this pull request Jul 20, 2020
This is an redesign attempt to solve the limitations explained in pyscaffold#271.
Currently the API is suffering from primitive obsession (a part of it was a
deliberate design choice to make the extension mechanism easily
understandable even for beginners).

The changes introduced here improve a bit, making the design more flexible
by replacing `FileOp` integer flags with functions.
Most of the primitive obsession is kept due to the same design decisions that
motivated the first implementation.

The following commits should ensure no errors when running PyScaffold and
add documentation.

Closes pyscaffold#271.
abravalheri added a commit to abravalheri/pyscaffold that referenced this pull request Jul 20, 2020
This is an redesign attempt to solve the limitations explained in pyscaffold#271.
Currently the API is suffering from primitive obsession (a part of it was a
deliberate design choice to make the extension mechanism easily
understandable even for beginners).

The changes introduced here improve a bit, making the design more flexible
by replacing `FileOp` integer flags with functions.
Most of the primitive obsession is kept due to the same design decisions that
motivated the first implementation.

The following commits should ensure no errors when running PyScaffold and
add documentation.

Closes pyscaffold#271.
abravalheri added a commit that referenced this pull request Jul 24, 2020
This is an redesign attempt to solve the limitations explained in #271.
Currently the API is suffering from primitive obsession (a part of it was a
deliberate design choice to make the extension mechanism easily
understandable even for beginners).

The changes introduced here improve a bit, making the design more flexible
by replacing `FileOp` integer flags with functions.
Most of the primitive obsession is kept due to the same design decisions that
motivated the first implementation.

The following commits should ensure no errors when running PyScaffold and
add documentation.

Closes #271.
@abravalheri
Copy link
Collaborator

#294 solves this issue

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.

None yet

3 participants