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

Features/improved dsl #77

Closed

Conversation

AdrieanKhisbe
Copy link
Collaborator

Follow up on #53.

Add run and async commands. (plus code helper functions)

Some other changes might be considered before merging:

  • check didn't break the vimshebang
  • clean function
  • assigns function
  • after/before (coule and maybe shoud be handled in other pr)
  • others evocated on run/async commands #53

@AdrieanKhisbe
Copy link
Collaborator Author

note to self: lint recommandation to apply

@AdrieanKhisbe
Copy link
Collaborator Author

@rylnd should we do a dedicated branch on your repo so we can be several to collaborate, and do incremental PR. (branch we will merge back into trunk once we have settle what we want to add to the dsl.

@AdrieanKhisbe
Copy link
Collaborator Author

just rebase on top of latest PR merged

@rylnd
Copy link
Owner

rylnd commented Apr 2, 2017

@AdrieanKhisbe I like the idea behind iok and iwarn (although maybe green, yellow and red, isuccess, iwarning, and ifailure), but I'm not convinced of the value of the other functions:

  • clean seems dangerous. It's just an alias to rm without telling (warning may be more appropriate) the user that it's an alias to rm. As a user, if a library provides me with such a function, I would expect it to do something in addition to the builtin. As it stands, its only purpose (to me) is to make me type more characters and potentially forget that it's really rm. If your intention was to expand its responsibility in the future, I would rather wait until that functionality is there.

  • load is similar to clean in my mind; it's a thin wrapper around . that (again, in my opinion) only serves to obfuscate what's actually happening. The same "if this was meant to do more, let's merge it when it does more" caveat applies here.

  • run and async have similar notes to those above.

In general, my reaction to these functions is that until the advantages provided by the function outweigh the obfuscation caused by using them, we shouldn't force users to use them (nor clutter the codebase with them). What I would prefer (for now) is that the tests that inspired/necessitated those helpers be documented in our examples section. This would provide both a list of common patterns for users to build on, and it would also help us document common use cases and determine things like:

  • what tasks work well
  • what tasks takes a lot of code to accomplish
  • what tasks are wholly illegible

@AdrieanKhisbe
Copy link
Collaborator Author

I understand your concern.

The point about the run, async, clean, load is that we don't run command directly.
Which will ease the pending test implementation.
This would also enable to add feature on the go of the function, printing the command for instance.

@AdrieanKhisbe
Copy link
Collaborator Author

@rylnd
Do you want me to try to implement the xit/xdescribe feature, along with the debug, verbose mode.
(the two big features that would be enable by the commands)

@AdrieanKhisbe
Copy link
Collaborator Author

(oups, bad manipulaation for the request review)

@AdrieanKhisbe
Copy link
Collaborator Author

Closing with lack of interest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants