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

Proposal: Change name of decorator "Wove" to something better #67

Closed
matthewadams opened this issue Oct 10, 2018 · 8 comments
Closed

Proposal: Change name of decorator "Wove" to something better #67

matthewadams opened this issue Oct 10, 2018 · 8 comments

Comments

@matthewadams
Copy link
Collaborator

I find the name Wove nonintuitive for that decorator. The English simple past participle wove doesn't really fit here and causes it to read awkwardly.

Better names, IMHO, include (but are not necessarily limited to):

  • Advised
  • Woven

Once a better name is chosen, backward compatibility could be provided by aliasing Wove and deprecating its usage in favor of the new name.

@mgechev
Copy link
Owner

mgechev commented Oct 10, 2018

I like Advised. Feel free to open a PR with the change.

@matthewadams
Copy link
Collaborator Author

This is lower priority for me, so I'll submit when I have time. In the meantime, I'm doing the following:

import { Wove as Advised } from 'aspect.js'
...
@Advised()
export default class Thing { // ...

@matthewadams matthewadams changed the title Change name of decorator "Wove" to something better Proposal: Change name of decorator "Wove" to something better Oct 16, 2018
@matthewadams
Copy link
Collaborator Author

After adding a few issues, PRs, etc, I've noticed that the term "wove" is used incorrectly in many places throughout the codebase. This issue should probably be increased in scope to address all usages of the word "wove".

@mgechev
Copy link
Owner

mgechev commented Nov 18, 2018

I'd suggest releasing 0.8.0 once we have this issue resolved.

@mgechev
Copy link
Owner

mgechev commented Nov 18, 2018

If you find anything else which requires refactoring, feel free to open a PR.

@matthewadams
Copy link
Collaborator Author

@mgechev, These changes in ffa52fa break backward compatibility AFAICT. I was assuming that this project abides by [semver](https://www.semver.org] versioning semantics, so a breaking change necessitates a major version bump.

I'm not sure what you mean by "I'd suggest releasing 0.8.0 once we have this issue resolved." I feel that you should release 0.8.0 now, because it adds a new feature & fixes a bug, but is otherwise backward compatible with 0.7.5. I feel that these changes should cause the version to go to 1.0.0.

@mgechev
Copy link
Owner

mgechev commented Nov 18, 2018

Yes, I wrote this comment before this one. Once we're happy with the current state of aspect.js, I'll publish 1.0.0.

@matthewadams
Copy link
Collaborator Author

Ah, ok, gotcha. :)

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