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

maintenance: consistency in variables names #1628

Closed
bartekleon opened this issue Jul 9, 2019 · 4 comments
Closed

maintenance: consistency in variables names #1628

bartekleon opened this issue Jul 9, 2019 · 4 comments

Comments

@bartekleon
Copy link
Member

Another maintenance idea:
One naming convention in variables names.

It may be pretty easy to execute since in #1615 I have added plugin for it.
Options are:

      { "type": "default", "format": "", "leadingUnderscore": "allow", "trailingUnderscore": "allow" },
      { "type": "variable", "modifiers": ["global", "const"], "format": "" },
      { "type": "variable", "modifiers": ["export", "const"], "format": "" },
      { "type": "functionVariable", "modifiers": ["export", "const"], "format": "" },
      { "type": "parameter", "modifiers": "unused", "leadingUnderscore": "allow" },
      { "type": "member", "modifiers": "private", "leadingUnderscore": "allow" },
      { "type": "member", "modifiers": "protected", "leadingUnderscore": "allow" },
      { "type": "method", "filter": "^toJSON$", "format": null },
      { "type": "property", "modifiers": ["public", "static", "const"], "format": "" },
      { "type": "type", "format": "PascalCase" },
      { "type": "class", "modifiers": "abstract", "prefix": "" },
      { "type": "interface", "prefix": "" },
      { "type": "genericTypeParameter", "prefix": "" },
      { "type": "enumMember", "format": "PascalCase" }

More info: https://github.com/ajafff/tslint-consistent-codestyle/blob/master/docs/naming-convention.md

@nicojs
Copy link
Member

nicojs commented Jul 12, 2019

Great! Please create a PR for this.

Just to confirm, the following will be valid variable names:

class Foo {
   private bar;
}
export const ALL_CAPS_ALLOWED_WHEN_CONST_IS_EXPORTED = 42;
const cammelCaseLocalConst = 43;
function cammelCaseFunction() {}
interface PascalInterface {
}
type PascalType = {
}
enum PascalEnum {
  Baz
}

The following are invalid:

class foo {
   private Bar;
}
function PascalCaseFunction() {}
interface cammelCaseInterface {
}
type cammelCaseType = {
}
enum cammelCase {
   baz
}

@bartekleon
Copy link
Member Author

Looks nice :) I will be able to make PR for that soon.

@bartekleon
Copy link
Member Author

bartekleon commented Jul 14, 2019

@nicojs I have some problems with private methods. Ex in File.ts you are using getters and so I can't do:

export default class File {
  constructor(public readonly name: string, content: Buffer | string) {
    if (typeof content === 'string') {
      this.content = Buffer.from(content);
      this.textContent = content;
    } else {
      this.content = content;
      this.textContent = content.toString();
    }
  }
  get content(): Buffer {
    return this.content;
  }
  get textContent(): string {
    return this.textContent;
  }
}

Coz I don't have setter: 'Cannot assign to 'textContent' because it is a read-only property'

Any idea how to fix that? Should I add setter? If yes, wouldn't that mean it can stay as a public variable?
Like new File(sth, sth).content gives value and new File(sth, sth).content = sth sets value
and
new File(sth, sth).content() gives value and new File(sth, sth).content(sth) sets value

@nicojs
Copy link
Member

nicojs commented Jul 15, 2019

What's the problem? Probably _content and _textContent aren't allowed? Change the name to innerContent and innerTextContent respectively.

// File.ts
/**
 * Represents a file within Stryker. Could be a strictly in-memory file.
 */
export default class File {

  private innerTextContent: string | undefined;
  private readonly innerContent: Buffer;

  /**
   * Creates a new File to be used within Stryker.
   * @param name The full name of the file (inc path)
   * @param content The buffered or string content of the file
   */
  constructor(public readonly name: string, content: Buffer | string) {
    if (typeof content === 'string') {
      this.innerContent = Buffer.from(content);
      this.innerTextContent = content;
    } else {
      this.innerContent = content;
    }
  }

  /**
   * Gets the underlying content as buffer.
   */
  get content(): Buffer {
    return this.innerContent;
  }

  /**
   * Gets the underlying content as string using utf8 encoding.
   */
  get textContent(): string {
    if (!this.innerTextContent ) {
      this.innerContent = this.content.toString();
    }
    return this.innerTextContent ;
  }
}

Any idea how to fix that? Should I add setter?

Most definitely not. I want File to be readonly. Having them writable might lead to obvious bugs.

 get content(): Buffer {
     return this.content;
   }

This results in a StackOverflowError 🤓

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 a pull request may close this issue.

2 participants