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

newspaper-order ignores member-ordering rule (e.g. private after public) mutually excluding themselves. #26

Open
adlh opened this issue Dec 2, 2017 · 3 comments
Assignees

Comments

@adlh
Copy link

adlh commented Dec 2, 2017

No description provided.

@adlh adlh changed the title newspaper-order ignores member-ordering rule (e.g. private after public) and they mutually exclude themselves. newspaper-order ignores member-ordering rule (e.g. private after public) mutually excluding themselves. Dec 2, 2017
@Glavin001 Glavin001 added the bug label Dec 2, 2017
@Glavin001
Copy link
Owner

Please provide a quick example.

Even better would be adding a test to https://github.com/Glavin001/tslint-clean-code/blob/master/src/tests/NewspaperOrderRuleTests.ts .

Thanks in advance!

adlh added a commit to adlh/tslint-clean-code that referenced this issue Dec 6, 2017
@adlh
Copy link
Author

adlh commented Dec 6, 2017

@Glavin001 ok, I added a test that demonstrates the problem. Hier is the commit

It took me a while to understand the problem and write an isolated test :-) ... So the problem is, when the first 2 public methods (get and set use a protected method getField) and then between those public methods and the protected getField, there is another public method. The rule then wants to move getField before that 3d public method:

  1) newspaperOrderRule ClassDeclaration should respect member-order when defining order:
     AssertionError: Wrong # of failures: 
[
  {
    "failure": "The class does not read like a Newspaper. Reorder the methods of the class: HasProtectedFieldsClass\n\nMethods order:\n1. ✓ get\n2. ✓ set\n3. x getField\n4. x anotherPublicMethod",
    "name": "file.ts",
    "ruleName": "newspaper-order",
    "ruleSeverity": "ERROR",
    "startPosition": {
      "character": 17,
      "line": 15
    }
  }
]: expected 1 to equal 0

@Glavin001
Copy link
Owner

🎉 This is great, thank you!

I looked at TSLint and could not figure out how to have one rule acknowledge another rule being enabled.

I think the best route may be to add configuration options for newspaper-order rule. Looking at https://palantir.github.io/tslint/rules/member-ordering/ there are a lot of options / possible configurations to support.

The applicable place to make a change would be around closestList: https://github.com/Glavin001/tslint-clean-code/blob/master/src/newspaperOrderRule.ts#L358-L377 . Instead of only trying to find the list of methods which results in minimal changes, also consider the member ordering configuration to filter out undesirable orders.
This would also mean we need a function which can look up the access modifiers (e.g. public, private) for a method name.

I won't have time to investigate this in great detail until the weekend. If you'd like to continue working on a fix in a Pull Request, let me know! Pull Requests are very welcome! 😃

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

No branches or pull requests

2 participants