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

Proposed new process standard for workarounds in code #92

Open
coreyshuman opened this issue Jan 31, 2019 · 9 comments
Open

Proposed new process standard for workarounds in code #92

coreyshuman opened this issue Jan 31, 2019 · 9 comments
Assignees

Comments

@coreyshuman
Copy link
Member

I would like to propose a new Shift3 rule: If you add code that is a workaround, you should include a comment which explains why it's there, and links to an official Github issue.

We have an example in our Normalize app, where navigation doesn't work correctly when simultaneously closing a modal window. Under this rule, all instances of the workaround would look something like this:

// This is a workaround for a navigation issue when closing a modal:
// https://github.com/NativeScript/nativescript-angular/issues/1380 
return setTimeout(() => Helpers.getRoute("badges", this.router), 50);

This will avoid confusion during code review and during future maintenance. This also documents the issue, so the workaround reasoning is not forgotten in the future.

@carlosvargas
Copy link
Contributor

carlosvargas commented Feb 1, 2019

I second this. While we should add an official Github issue if there is one, I would suggest that we should also add any Stack Overflow or blog post links that were used to come up with the workaround, since most of the time these links have more documentation about the actual issue than the Github issue itself.

I would also extend this to any overly complicated code that we didn't write. For example:

// Calculate the Luminance of a color so that we can 
// figure out if we should use a light or dark background
// see: http://stackoverflow.com/a/3943023/112731
function getLuminance(c: RgbArray): number {
    let i, x;
    const a = []; // so we don't mutate
    for (i = 0; i < c.length; i++) {
        x = c[i] / 255;
        a[i] = x <= 0.03928 ? x / 12.92 : Math.pow((x + 0.055) / 1.055, 2.4);
    }
    return 0.2126 * a[0] + 0.7152 * a[1] + 0.0722 * a[2];
}

@coreyshuman
Copy link
Member Author

Oh yeah I like extending this rule to cover magic functions we find online.

@kmausolf
Copy link
Contributor

kmausolf commented Feb 1, 2019

I'd like to add that you should always cite when you use someone's workaround that you found online.

@mwallert
Copy link
Contributor

mwallert commented Feb 1, 2019

Yeah these are all good, sometimes we have a lot of "magic" we get from elsewhere which can create confusion when it needs to be adjusted/used elsewhere. @coreyshuman I'm thinking now we create a Shift3 Rules to Code By folder in the root and link it in the base readme. Thoughts everyone?

@ryekerjh
Copy link
Contributor

ryekerjh commented Feb 1, 2019

@mwallert I agree, we should have a designated folder for this stuff with a basic README.md to explain. It will grow over time time sure.

@coreyshuman
Copy link
Member Author

@mwallert @ryekerjh I think that's a great idea. We should make sure the new employee on-boarding / getting started hits that Rules to Code By folder.

@ryekerjh
Copy link
Contributor

ryekerjh commented Feb 6, 2019

@coreyshuman I added issue #95 for this

@coreyshuman
Copy link
Member Author

@mwallert @ryekerjh I'm continuing to find things that don't quite need a full dedicated file, but would be considered standards. Instead of a Rules to Code By folder, I think this could be a single file with important small items such as:

  • Use Flow in React
  • Use Typescript in Node
  • Put business login in the server
  • Write unit tests for your business logic
  • add a comment with link to resources when implementing a workaround or magic function

Anyone else have opinions?

@kmausolf
Copy link
Contributor

I feel like this is the situation where we need a Wiki. Separating these out into folders is nice, because I can quickly find what I'm looking for, but having them all in one file prevents us from having to maintain a ton of different folders. A Wiki for this would benefit both ends of this.

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

6 participants