diff options
author | Brett Walker <bwalker@gitlab.com> | 2018-09-06 16:52:18 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2018-09-06 16:52:18 +0000 |
commit | b0be58a1b313df976ea4b0e37163f8fea81ce5f4 (patch) | |
tree | 9f24d3c53f6d47fb6266f00db9f60f514447ed2a /doc/development | |
parent | 2d16f4794b43966595e8c6b6405b55c60e94e866 (diff) | |
download | gitlab-ce-b0be58a1b313df976ea4b0e37163f8fea81ce5f4.tar.gz |
Resolve "CE documentation is not CommonMark compliant"
Diffstat (limited to 'doc/development')
20 files changed, 682 insertions, 637 deletions
diff --git a/doc/development/automatic_ce_ee_merge.md b/doc/development/automatic_ce_ee_merge.md index 8d41503f874..f6509b5c1dd 100644 --- a/doc/development/automatic_ce_ee_merge.md +++ b/doc/development/automatic_ce_ee_merge.md @@ -9,16 +9,16 @@ This merge is done automatically in a ## What to do if you are pinged in a `CE Upstream` merge request to resolve a conflict? 1. Please resolve the conflict as soon as possible or ask someone else to do it - - It's ok to resolve more conflicts than the one that you are asked to resolve. - In that case, it's a good habit to ask for a double-check on your resolution - by someone who is familiar with the code you touched. + - It's ok to resolve more conflicts than the one that you are asked to resolve. + In that case, it's a good habit to ask for a double-check on your resolution + by someone who is familiar with the code you touched. 1. Once you have resolved your conflicts, push to the branch (no force-push) 1. Assign the merge request to the next person that has to resolve a conflict 1. If all conflicts are resolved after your resolution is pushed, keep the merge - request assigned to you: **you are now responsible for the merge request to be - green** + request assigned to you: **you are now responsible for the merge request to be + green** 1. If you need any help, you can ping the current [release managers], or ask in - the `#ce-to-ee` Slack channel + the `#ce-to-ee` Slack channel A few notes about the automatic CE->EE merge job: @@ -127,19 +127,19 @@ Now, every time you create an MR for CE and EE: 1. Open two terminal windows, one in CE, and another one in EE 1. In the CE terminal: - 1. Create the CE branch, e.g., `branch-example` - 1. Make your changes and push a commit (commit A) - 1. Create the CE merge request in GitLab + 1. Create the CE branch, e.g., `branch-example` + 1. Make your changes and push a commit (commit A) + 1. Create the CE merge request in GitLab 1. In the EE terminal: - 1. Create the EE-equivalent branch ending with `-ee`, e.g., - `git checkout -b branch-example-ee` - 1. Fetch the CE branch: `git fetch ce branch-example` - 1. Cherry-pick the commit A: `git cherry-pick commit-A-SHA` - 1. If Git prompts you to fix the conflicts, do a `git status` - to check which files contain conflicts, fix them, save the files - 1. Add the changes with `git add .` but **DO NOT commit** them - 1. Continue cherry-picking: `git cherry-pick --continue` - 1. Push to EE: `git push origin branch-example-ee` + 1. Create the EE-equivalent branch ending with `-ee`, e.g., + `git checkout -b branch-example-ee` + 1. Fetch the CE branch: `git fetch ce branch-example` + 1. Cherry-pick the commit A: `git cherry-pick commit-A-SHA` + 1. If Git prompts you to fix the conflicts, do a `git status` + to check which files contain conflicts, fix them, save the files + 1. Add the changes with `git add .` but **DO NOT commit** them + 1. Continue cherry-picking: `git cherry-pick --continue` + 1. Push to EE: `git push origin branch-example-ee` 1. Create the EE-equivalent MR and link to the CE MR from the description "Ports [CE-MR-LINK] to EE" 1. Once all the jobs are passing in both CE and EE, you've addressed the diff --git a/doc/development/background_migrations.md b/doc/development/background_migrations.md index f0d5af9fcb5..bb9a296ef12 100644 --- a/doc/development/background_migrations.md +++ b/doc/development/background_migrations.md @@ -10,10 +10,10 @@ migrations automatically reschedule themselves for a later point in time. ## When To Use Background Migrations ->**Note:** -When adding background migrations _you must_ make sure they are announced in the -monthly release post along with an estimate of how long it will take to complete -the migrations. +> **Note:** +> When adding background migrations _you must_ make sure they are announced in the +> monthly release post along with an estimate of how long it will take to complete +> the migrations. In the vast majority of cases you will want to use a regular Rails migration instead. Background migrations should _only_ be used when migrating _data_ in @@ -127,23 +127,23 @@ big JSON blob) to column `bar` (containing a string). The process for this would roughly be as follows: 1. Release A: - 1. Create a migration class that perform the migration for a row with a given ID. - 1. Deploy the code for this release, this should include some code that will - schedule jobs for newly created data (e.g. using an `after_create` hook). - 1. Schedule jobs for all existing rows in a post-deployment migration. It's - possible some newly created rows may be scheduled twice so your migration - should take care of this. + 1. Create a migration class that perform the migration for a row with a given ID. + 1. Deploy the code for this release, this should include some code that will + schedule jobs for newly created data (e.g. using an `after_create` hook). + 1. Schedule jobs for all existing rows in a post-deployment migration. It's + possible some newly created rows may be scheduled twice so your migration + should take care of this. 1. Release B: - 1. Deploy code so that the application starts using the new column and stops - scheduling jobs for newly created data. - 1. In a post-deployment migration you'll need to ensure no jobs remain. - 1. Use `Gitlab::BackgroundMigration.steal` to process any remaining - jobs in Sidekiq. - 1. Reschedule the migration to be run directly (i.e. not through Sidekiq) - on any rows that weren't migrated by Sidekiq. This can happen if, for - instance, Sidekiq received a SIGKILL, or if a particular batch failed - enough times to be marked as dead. - 1. Remove the old column. + 1. Deploy code so that the application starts using the new column and stops + scheduling jobs for newly created data. + 1. In a post-deployment migration you'll need to ensure no jobs remain. + 1. Use `Gitlab::BackgroundMigration.steal` to process any remaining + jobs in Sidekiq. + 1. Reschedule the migration to be run directly (i.e. not through Sidekiq) + on any rows that weren't migrated by Sidekiq. This can happen if, for + instance, Sidekiq received a SIGKILL, or if a particular batch failed + enough times to be marked as dead. + 1. Remove the old column. This may also require a bump to the [import/export version][import-export], if importing a project from a prior version of GitLab requires the data to be in diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 23c80799235..e50e6370c80 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -5,30 +5,30 @@ There are a few rules to get your merge request accepted: 1. Your merge request should only be **merged by a [maintainer][team]**. - 1. If your merge request includes only backend changes [^1], it must be - **approved by a [backend maintainer][projects]**. - 1. If your merge request includes only frontend changes [^1], it must be - **approved by a [frontend maintainer][projects]**. - 1. If your merge request includes UX changes [^1], it must - be **approved by a [UX team member][team]**. - 1. If your merge request includes adding a new JavaScript library [^1], it must be - **approved by a [frontend lead][team]**. - 1. If your merge request includes adding a new UI/UX paradigm [^1], it must be - **approved by a [UX lead][team]**. - 1. If your merge request includes frontend and backend changes [^1], it must - be **approved by a [frontend and a backend maintainer][projects]**. - 1. If your merge request includes UX and frontend changes [^1], it must - be **approved by a [UX team member and a frontend maintainer][team]**. - 1. If your merge request includes UX, frontend and backend changes [^1], it must - be **approved by a [UX team member, a frontend and a backend maintainer][team]**. - 1. If your merge request includes a new dependency or a filesystem change, it must - be *approved by a [Distribution team member][team]*. See how to work with the [Distribution team for more details.](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) + 1. If your merge request includes only backend changes [^1], it must be + **approved by a [backend maintainer][projects]**. + 1. If your merge request includes only frontend changes [^1], it must be + **approved by a [frontend maintainer][projects]**. + 1. If your merge request includes UX changes [^1], it must + be **approved by a [UX team member][team]**. + 1. If your merge request includes adding a new JavaScript library [^1], it must be + **approved by a [frontend lead][team]**. + 1. If your merge request includes adding a new UI/UX paradigm [^1], it must be + **approved by a [UX lead][team]**. + 1. If your merge request includes frontend and backend changes [^1], it must + be **approved by a [frontend and a backend maintainer][projects]**. + 1. If your merge request includes UX and frontend changes [^1], it must + be **approved by a [UX team member and a frontend maintainer][team]**. + 1. If your merge request includes UX, frontend and backend changes [^1], it must + be **approved by a [UX team member, a frontend and a backend maintainer][team]**. + 1. If your merge request includes a new dependency or a filesystem change, it must + be *approved by a [Distribution team member][team]*. See how to work with the [Distribution team for more details.](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) 1. To lower the amount of merge requests maintainers need to review, you can - ask or assign any [reviewers][projects] for a first review. - 1. If you need some guidance (e.g. it's your first merge request), feel free - to ask one of the [Merge request coaches][team]. - 1. The reviewer will assign the merge request to a maintainer once the - reviewer is satisfied with the state of the merge request. + ask or assign any [reviewers][projects] for a first review. + 1. If you need some guidance (e.g. it's your first merge request), feel free + to ask one of the [Merge request coaches][team]. + 1. The reviewer will assign the merge request to a maintainer once the + reviewer is satisfied with the state of the merge request. 1. Keep in mind that maintainers are also going to perform a final code review. The ideal scenario is that the reviewer has already addressed any concerns the maintainer would have found, and the maintainer only has to perform the @@ -160,41 +160,41 @@ Enterprise Edition instance. This has some implications: 1. **Query changes** should be tested to ensure that they don't result in worse performance at the scale of GitLab.com: - 1. Generating large quantities of data locally can help. - 2. Asking for query plans from GitLab.com is the most reliable way to validate - these. + 1. Generating large quantities of data locally can help. + 2. Asking for query plans from GitLab.com is the most reliable way to validate + these. 2. **Database migrations** must be: - 1. Reversible. - 2. Performant at the scale of GitLab.com - ask a maintainer to test the - migration on the staging environment if you aren't sure. - 3. Categorised correctly: - - Regular migrations run before the new code is running on the instance. - - [Post-deployment migrations](post_deployment_migrations.md) run _after_ - the new code is deployed, when the instance is configured to do that. - - [Background migrations](background_migrations.md) run in Sidekiq, and - should only be done for migrations that would take an extreme amount of - time at GitLab.com scale. + 1. Reversible. + 2. Performant at the scale of GitLab.com - ask a maintainer to test the + migration on the staging environment if you aren't sure. + 3. Categorised correctly: + - Regular migrations run before the new code is running on the instance. + - [Post-deployment migrations](post_deployment_migrations.md) run _after_ + the new code is deployed, when the instance is configured to do that. + - [Background migrations](background_migrations.md) run in Sidekiq, and + should only be done for migrations that would take an extreme amount of + time at GitLab.com scale. 3. **Sidekiq workers** [cannot change in a backwards-incompatible way](sidekiq_style_guide.md#removing-or-renaming-queues): - 1. Sidekiq queues are not drained before a deploy happens, so there will be - workers in the queue from the previous version of GitLab. - 2. If you need to change a method signature, try to do so across two releases, - and accept both the old and new arguments in the first of those. - 3. Similarly, if you need to remove a worker, stop it from being scheduled in - one release, then remove it in the next. This will allow existing jobs to - execute. - 4. Don't forget, not every instance will upgrade to every intermediate version - (some people may go from X.1.0 to X.10.0, or even try bigger upgrades!), so - try to be liberal in accepting the old format if it is cheap to do so. + 1. Sidekiq queues are not drained before a deploy happens, so there will be + workers in the queue from the previous version of GitLab. + 2. If you need to change a method signature, try to do so across two releases, + and accept both the old and new arguments in the first of those. + 3. Similarly, if you need to remove a worker, stop it from being scheduled in + one release, then remove it in the next. This will allow existing jobs to + execute. + 4. Don't forget, not every instance will upgrade to every intermediate version + (some people may go from X.1.0 to X.10.0, or even try bigger upgrades!), so + try to be liberal in accepting the old format if it is cheap to do so. 4. **Cached values** may persist across releases. If you are changing the type a cached value returns (say, from a string or nil to an array), change the cache key at the same time. 5. **Settings** should be added as a [last resort](https://about.gitlab.com/handbook/product/#convention-over-configuration). If you're adding a new setting in `gitlab.yml`: - 1. Try to avoid that, and add to `ApplicationSetting` instead. - 2. Ensure that it is also - [added to Omnibus](https://docs.gitlab.com/omnibus/settings/gitlab.yml.html#adding-a-new-setting-to-gitlab-yml). + 1. Try to avoid that, and add to `ApplicationSetting` instead. + 2. Ensure that it is also + [added to Omnibus](https://docs.gitlab.com/omnibus/settings/gitlab.yml.html#adding-a-new-setting-to-gitlab-yml). 6. **Filesystem access** can be slow, so try to avoid [shared files](shared_files.md) when an alternative solution is available. diff --git a/doc/development/contributing/index.md b/doc/development/contributing/index.md index 64f5a2c8022..eac7cb44c40 100644 --- a/doc/development/contributing/index.md +++ b/doc/development/contributing/index.md @@ -16,7 +16,7 @@ - [Milestone labels](#milestone-labels) - [Bug Priority labels](#bug-priority-labels) - [Bug Severity labels](#bug-severity-labels) - - [Severity impact guidance](#severity-impact-guidance) + - [Severity impact guidance](#severity-impact-guidance) - [Label for community contributors](#label-for-community-contributors) - [Implement design & UI elements](#implement-design--ui-elements) - [Issue tracker](#issue-tracker) diff --git a/doc/development/contributing/issue_workflow.md b/doc/development/contributing/issue_workflow.md index 6a334e9b17d..08042fa2aec 100644 --- a/doc/development/contributing/issue_workflow.md +++ b/doc/development/contributing/issue_workflow.md @@ -9,7 +9,7 @@ - [Release Scoping labels](#release-scoping-labels) - [Priority labels](#priority-labels) - [Severity labels](#severity-labels) - - [Severity impact guidance](#severity-impact-guidance) + - [Severity impact guidance](#severity-impact-guidance) - [Label for community contributors](#label-for-community-contributors) - [Issue triaging](#issue-triaging) - [Feature proposals](#feature-proposals) @@ -250,7 +250,7 @@ code snippet right after your description in a new line: `~"feature proposal"`. Please keep feature proposals as small and simple as possible, complex ones might be edited to make them small and simple. -Please submit Feature Proposals using the ['Feature Proposal' issue template](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab/issue_templates/Feature Proposal.md) provided on the issue tracker. +Please submit Feature Proposals using the ['Feature Proposal' issue template](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab/issue_templates/Feature%20Proposal.md) provided on the issue tracker. For changes in the interface, it is helpful to include a mockup. Issues that add to, or change, the interface should be given the ~"UX" label. This will allow the UX team to provide input and guidance. You may diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index 9b1da4e7bc1..685287f7a0c 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -55,30 +55,30 @@ request is as follows: organized commits by [squashing them][git-squash] 1. Push the commit(s) to your fork 1. Submit a merge request (MR) to the `master` branch - 1. Your merge request needs at least 1 approval but feel free to require more. - For instance if you're touching backend and frontend code, it's a good idea - to require 2 approvals: 1 from a backend maintainer and 1 from a frontend - maintainer - 1. You don't have to select any approvers, but you can if you really want - specific people to approve your merge request + 1. Your merge request needs at least 1 approval but feel free to require more. + For instance if you're touching backend and frontend code, it's a good idea + to require 2 approvals: 1 from a backend maintainer and 1 from a frontend + maintainer + 1. You don't have to select any approvers, but you can if you really want + specific people to approve your merge request 1. The MR title should describe the change you want to make 1. The MR description should give a motive for your change and the method you used to achieve it. - 1. If you are contributing code, fill in the template already provided in the - "Description" field. - 1. If you are contributing documentation, choose `Documentation` from the - "Choose a template" menu and fill in the template. - 1. Mention the issue(s) your merge request solves, using the `Solves #XXX` or - `Closes #XXX` syntax to auto-close the issue(s) once the merge request will - be merged. + 1. If you are contributing code, fill in the template already provided in the + "Description" field. + 1. If you are contributing documentation, choose `Documentation` from the + "Choose a template" menu and fill in the template. + 1. Mention the issue(s) your merge request solves, using the `Solves #XXX` or + `Closes #XXX` syntax to auto-close the issue(s) once the merge request will + be merged. 1. If you're allowed to, set a relevant milestone and labels 1. If the MR changes the UI it should include *Before* and *After* screenshots 1. If the MR changes CSS classes please include the list of affected pages, `grep css-class ./app -R` 1. Be prepared to answer questions and incorporate feedback even if requests for this arrive weeks or months after your MR submission - 1. If a discussion has been addressed, select the "Resolve discussion" button - beneath it to mark it resolved. + 1. If a discussion has been addressed, select the "Resolve discussion" button + beneath it to mark it resolved. 1. If your MR touches code that executes shell commands, reads or opens files or handles paths to files on disk, make sure it adheres to the [shell command guidelines](../shell_commands.md) @@ -138,7 +138,7 @@ When having your code reviewed and when reviewing merge requests please take the making and testing future changes 1. Changes do not adversely degrade performance. - Avoid repeated polling of endpoints that require a significant amount of overhead - - Check for N+1 queries via the SQL log or [`QueryRecorder`](https://docs.gitlab.com/ce/development/mer ge_request_performance_guidelines.html) + - Check for N+1 queries via the SQL log or [`QueryRecorder`](https://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html) - Avoid repeated access of filesystem 1. If you need polling to support real-time features, please use [polling with ETag caching][polling-etag]. diff --git a/doc/development/diffs.md b/doc/development/diffs.md index 725507b7ef6..2738b1b5635 100644 --- a/doc/development/diffs.md +++ b/doc/development/diffs.md @@ -26,11 +26,11 @@ In order to present diffs information on the Merge Request diffs page, we: 1. Fetch all diff files from database `merge_request_diff_files` 2. Fetch the _old_ and _new_ file blobs in batch to: - 1. Highlight old and new file content - 2. Know which viewer it should use for each file (text, image, deleted, etc) - 3. Know if the file content changed - 4. Know if it was stored externally - 5. Know if it had storage errors + 1. Highlight old and new file content + 2. Know which viewer it should use for each file (text, image, deleted, etc) + 3. Know if the file content changed + 4. Know if it was stored externally + 5. Know if it had storage errors 3. If the diff file is cacheable (text-based), it's cached on Redis using `Gitlab::Diff::FileCollection::MergeRequestDiff` diff --git a/doc/development/documentation/styleguide.md b/doc/development/documentation/styleguide.md index 6c60a517b6d..e610c7595a8 100644 --- a/doc/development/documentation/styleguide.md +++ b/doc/development/documentation/styleguide.md @@ -297,10 +297,10 @@ avoid duplication, link to the special document that can be found in [`doc/administration/restart_gitlab.md`][doc-restart]. Usually the text will read like: - ``` - Save the file and [reconfigure GitLab](../../administration/restart_gitlab.md) - for the changes to take effect. - ``` +``` +Save the file and [reconfigure GitLab](../../administration/restart_gitlab.md) +for the changes to take effect. +``` If the document you are editing resides in a place other than the GitLab CE/EE `doc/` directory, instead of the relative link, use the full path: diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index 1cd873b6fe3..f9e6efa2c30 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -166,47 +166,53 @@ There are a few gotchas with it: to make it call the other method we want to extend, like a [template method pattern](https://en.wikipedia.org/wiki/Template_method_pattern). For example, given this base: - ``` ruby - class Base - def execute - return unless enabled? - # ... - # ... - end - end - ``` - Instead of just overriding `Base#execute`, we should update it and extract - the behaviour into another method: - ``` ruby - class Base - def execute - return unless enabled? - - do_something + ```ruby + class Base + def execute + return unless enabled? + + # ... + # ... + end end + ``` - private + Instead of just overriding `Base#execute`, we should update it and extract + the behaviour into another method: - def do_something - # ... - # ... + ```ruby + class Base + def execute + return unless enabled? + + do_something + end + + private + + def do_something + # ... + # ... + end end - end - ``` - Then we're free to override that `do_something` without worrying about the - guards: - ``` ruby - module EE::Base - extend ::Gitlab::Utils::Override - - override :do_something - def do_something - # Follow the above pattern to call super and extend it + ``` + + Then we're free to override that `do_something` without worrying about the + guards: + + ```ruby + module EE::Base + extend ::Gitlab::Utils::Override + + override :do_something + def do_something + # Follow the above pattern to call super and extend it + end end - end - ``` - This would require updating CE first, or make sure this is back ported to CE. + ``` + + This would require updating CE first, or make sure this is back ported to CE. When prepending, place them in the `ee/` specific sub-directory, and wrap class or module in `module EE` to avoid naming conflicts. @@ -469,7 +475,7 @@ Put the EE module files following For EE API routes, we put them in a `prepended` block: -``` ruby +```ruby module EE module API module MergeRequests @@ -503,7 +509,7 @@ interface first here. For example, suppose we have a few more optional params for EE, given this CE API code: -``` ruby +```ruby module API class MergeRequests < Grape::API # EE::API::MergeRequests would override the following helpers @@ -525,7 +531,7 @@ end And then we could override it in EE module: -``` ruby +```ruby module EE module API module MergeRequests @@ -552,7 +558,7 @@ To make it easy for an EE module to override the CE helpers, we need to define those helpers we want to extend first. Try to do that immediately after the class definition to make it easy and clear: -``` ruby +```ruby module API class JobArtifacts < Grape::API # EE::API::JobArtifacts would override the following helpers @@ -569,7 +575,7 @@ end And then we can follow regular object-oriented practices to override it: -``` ruby +```ruby module EE module API module JobArtifacts @@ -596,7 +602,7 @@ therefore can't be simply overridden. We need to extract them into a standalone method, or introduce some "hooks" where we could inject behavior in the CE route. Something like this: -``` ruby +```ruby module API class MergeRequests < Grape::API helpers do @@ -623,7 +629,7 @@ end Note that `update_merge_request_ee` doesn't do anything in CE, but then we could override it in EE: -``` ruby +```ruby module EE module API module MergeRequests @@ -662,7 +668,7 @@ For example, in one place we need to pass an extra argument to `at_least_one_of` so that the API could consider an EE-only argument as the least argument. This is not quite beautiful but it's working: -``` ruby +```ruby module API class MergeRequests < Grape::API def self.update_params_at_least_one_of @@ -683,7 +689,7 @@ end And then we could easily extend that argument in the EE class method: -``` ruby +```ruby module EE module API module MergeRequests diff --git a/doc/development/fe_guide/performance.md b/doc/development/fe_guide/performance.md index da836a0e82e..ef0eed786d2 100644 --- a/doc/development/fe_guide/performance.md +++ b/doc/development/fe_guide/performance.md @@ -74,7 +74,7 @@ bundle and included on the page. > `content_for :page_specific_javascripts` within haml files, along with > manually generated webpack bundles. However under this new system you should > not ever need to manually add an entry point to the `webpack.config.js` file. - +> > **Tip:** > If you are unsure what controller and action corresponds to a given page, you > can find this out by inspecting `document.body.dataset.page` within your @@ -109,7 +109,6 @@ bundle and included on the page. `my_widget.js` is only imported within `pages/widget/show/index.js`, you should place the module at `pages/widget/show/my_widget.js` and import it with a relative path (e.g. `import initMyWidget from './my_widget';`). - - If a class or module is _used by multiple routes_, place it within a shared directory at the closest common parent directory for the entry points that import it. For example, if `my_widget.js` is imported within diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 284b4b53334..656ddd868cd 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -17,74 +17,81 @@ at the top, but legacy files are a special case. Any time you develop a new fea refactor an existing one, you should abide by the eslint rules. 1. **Never Ever EVER** disable eslint globally for a file - ```javascript - // bad - /* eslint-disable */ - - // better - /* eslint-disable some-rule, some-other-rule */ - // best - // nothing :) - ``` + ```javascript + // bad + /* eslint-disable */ + + // better + /* eslint-disable some-rule, some-other-rule */ + + // best + // nothing :) + ``` 1. If you do need to disable a rule for a single violation, try to do it as locally as possible - ```javascript - // bad - /* eslint-disable no-new */ - - import Foo from 'foo'; - - new Foo(); - // better - import Foo from 'foo'; + ```javascript + // bad + /* eslint-disable no-new */ + + import Foo from 'foo'; + + new Foo(); + + // better + import Foo from 'foo'; + + // eslint-disable-next-line no-new + new Foo(); + ``` - // eslint-disable-next-line no-new - new Foo(); - ``` 1. There are few rules that we need to disable due to technical debt. Which are: - 1. [no-new][eslint-new] - 1. [class-methods-use-this][eslint-this] + 1. [no-new][eslint-new] + 1. [class-methods-use-this][eslint-this] 1. When they are needed _always_ place ESlint directive comment blocks on the first line of a script, followed by any global declarations, then a blank newline prior to any imports or code. - ```javascript - // bad - /* global Foo */ - /* eslint-disable no-new */ - import Bar from './bar'; - - // good - /* eslint-disable no-new */ - /* global Foo */ - import Bar from './bar'; - ``` + ```javascript + // bad + /* global Foo */ + /* eslint-disable no-new */ + import Bar from './bar'; + + // good + /* eslint-disable no-new */ + /* global Foo */ + + import Bar from './bar'; + ``` 1. **Never** disable the `no-undef` rule. Declare globals with `/* global Foo */` instead. 1. When declaring multiple globals, always use one `/* global [name] */` line per variable. - ```javascript - // bad - /* globals Flash, Cookies, jQuery */ - // good - /* global Flash */ - /* global Cookies */ - /* global jQuery */ - ``` + ```javascript + // bad + /* globals Flash, Cookies, jQuery */ + + // good + /* global Flash */ + /* global Cookies */ + /* global jQuery */ + ``` 1. Use up to 3 parameters for a function or class. If you need more accept an Object instead. - ```javascript - // bad - fn(p1, p2, p3, p4) {} - // good - fn(options) {} - ``` + ```javascript + // bad + fn(p1, p2, p3, p4) {} + + // good + fn(options) {} + ``` #### Modules, Imports, and Exports + 1. Use ES module syntax to import modules ```javascript // bad @@ -178,109 +185,116 @@ Do not use them anymore and feel free to remove them when refactoring legacy cod ``` #### Data Mutation and Pure functions + 1. Strive to write many small pure functions, and minimize where mutations occur. - ```javascript + + ```javascript // bad const values = {foo: 1}; - + function impureFunction(items) { const bar = 1; - + items.foo = items.a * bar + 2; - + return items.a; } - + const c = impureFunction(values); - + // good var values = {foo: 1}; - + function pureFunction (foo) { var bar = 1; - + foo = foo * bar + 2; - + return foo; } - + var c = pureFunction(values.foo); - ``` + ``` 1. Avoid constructors with side-effects. -Although we aim for code without side-effects we need some side-effects for our code to run. - -If the class won't do anything if we only instantiate it, it's ok to add side effects into the constructor (_Note:_ The following is just an example. If the only purpose of the class is to add an event listener and handle the callback a function will be more suitable.) - -```javascript -// Bad -export class Foo { - constructor() { - this.init(); - } - init() { - document.addEventListener('click', this.handleCallback) - }, - handleCallback() { - - } -} - -// Good -export class Foo { - constructor() { - document.addEventListener() - } - handleCallback() { - } -} -``` - -On the other hand, if a class only needs to extend a third party/add event listeners in some specific cases, they should be initialized outside of the constructor. + Although we aim for code without side-effects we need some side-effects for our code to run. -1. Prefer `.map`, `.reduce` or `.filter` over `.forEach` -A forEach will most likely cause side effects, it will be mutating the array being iterated. Prefer using `.map`, -`.reduce` or `.filter` - ```javascript - const users = [ { name: 'Foo' }, { name: 'Bar' } ]; + If the class won't do anything if we only instantiate it, it's ok to add side effects into the constructor (_Note:_ The following is just an example. If the only purpose of the class is to add an event listener and handle the callback a function will be more suitable.) - // bad - users.forEach((user, index) => { - user.id = index; - }); + ```javascript + // Bad + export class Foo { + constructor() { + this.init(); + } + init() { + document.addEventListener('click', this.handleCallback) + }, + handleCallback() { + + } + } + + // Good + export class Foo { + constructor() { + document.addEventListener() + } + handleCallback() { + } + } + ``` - // good - const usersWithId = users.map((user, index) => { - return Object.assign({}, user, { id: index }); - }); - ``` + On the other hand, if a class only needs to extend a third party/add event listeners in some specific cases, they should be initialized outside of the constructor. + +1. Prefer `.map`, `.reduce` or `.filter` over `.forEach` + A forEach will most likely cause side effects, it will be mutating the array being iterated. Prefer using `.map`, + `.reduce` or `.filter` + + ```javascript + const users = [ { name: 'Foo' }, { name: 'Bar' } ]; + + // bad + users.forEach((user, index) => { + user.id = index; + }); + + // good + const usersWithId = users.map((user, index) => { + return Object.assign({}, user, { id: index }); + }); + ``` #### Parse Strings into Numbers -1. `parseInt()` is preferable over `Number()` or `+` - ```javascript - // bad - +'10' // 10 - // good - Number('10') // 10 +1. `parseInt()` is preferable over `Number()` or `+` - // better - parseInt('10', 10); - ``` + ```javascript + // bad + +'10' // 10 + + // good + Number('10') // 10 + + // better + parseInt('10', 10); + ``` #### CSS classes used for JavaScript + 1. If the class is being used in Javascript it needs to be prepend with `js-` - ```html - // bad - <button class="add-user"> - Add User - </button> - // good - <button class="js-add-user"> - Add User - </button> - ``` + ```html + // bad + <button class="add-user"> + Add User + </button> + + // good + <button class="js-add-user"> + Add User + </button> + ``` ### Vue.js @@ -292,197 +306,211 @@ Please check this [rules][eslint-plugin-vue-rules] for more documentation. 1. The service has it's own file 1. The store has it's own file 1. Use a function in the bundle file to instantiate the Vue component: - ```javascript - // bad - class { - init() { - new Component({}) - } - } - // good - document.addEventListener('DOMContentLoaded', () => new Vue({ - el: '#element', - components: { - componentName - }, - render: createElement => createElement('component-name'), - })); - ``` + ```javascript + // bad + class { + init() { + new Component({}) + } + } + + // good + document.addEventListener('DOMContentLoaded', () => new Vue({ + el: '#element', + components: { + componentName + }, + render: createElement => createElement('component-name'), + })); + ``` 1. Do not use a singleton for the service or the store - ```javascript - // bad - class Store { - constructor() { - if (!this.prototype.singleton) { - // do something + + ```javascript + // bad + class Store { + constructor() { + if (!this.prototype.singleton) { + // do something + } } } - } - - // good - class Store { - constructor() { - // do something + + // good + class Store { + constructor() { + // do something + } } - } - ``` + ``` 1. Use `.vue` for Vue templates. Do not use `%template` in HAML. #### Naming 1. **Extensions**: Use `.vue` extension for Vue components. Do not use `.js` as file extension ([#34371]). 1. **Reference Naming**: Use PascalCase for their instances: - ```javascript - // bad - import cardBoard from 'cardBoard.vue' - components: { - cardBoard, - }; - - // good - import CardBoard from 'cardBoard.vue' - - components: { - CardBoard, - }; - ``` + ```javascript + // bad + import cardBoard from 'cardBoard.vue' + + components: { + cardBoard, + }; + + // good + import CardBoard from 'cardBoard.vue' + + components: { + CardBoard, + }; + ``` 1. **Props Naming:** Avoid using DOM component prop names. 1. **Props Naming:** Use kebab-case instead of camelCase to provide props in templates. - ```javascript - // bad - <component class="btn"> - - // good - <component css-class="btn"> - // bad - <component myProp="prop" /> - - // good - <component my-prop="prop" /> - ``` + ```javascript + // bad + <component class="btn"> + + // good + <component css-class="btn"> + + // bad + <component myProp="prop" /> + + // good + <component my-prop="prop" /> + ``` [#34371]: https://gitlab.com/gitlab-org/gitlab-ce/issues/34371 #### Alignment 1. Follow these alignment styles for the template method: - 1. With more than one attribute, all attributes should be on a new line: - ```javascript - // bad - <component v-if="bar" - param="baz" /> - - <button class="btn">Click me</button> - - // good - <component - v-if="bar" - param="baz" - /> - - <button class="btn"> - Click me - </button> - ``` - 1. The tag can be inline if there is only one attribute: - ```javascript - // good - <component bar="bar" /> - - // good - <component - bar="bar" - /> - // bad - <component - bar="bar" /> - ``` + 1. With more than one attribute, all attributes should be on a new line: + + ```javascript + // bad + <component v-if="bar" + param="baz" /> + + <button class="btn">Click me</button> + + // good + <component + v-if="bar" + param="baz" + /> + + <button class="btn"> + Click me + </button> + ``` + + 1. The tag can be inline if there is only one attribute: + + ```javascript + // good + <component bar="bar" /> + + // good + <component + bar="bar" + /> + + // bad + <component + bar="bar" /> + ``` #### Quotes + 1. Always use double quotes `"` inside templates and single quotes `'` for all other JS. - ```javascript - // bad - template: ` - <button :class='style'>Button</button> - ` - // good - template: ` - <button :class="style">Button</button> - ` - ``` + ```javascript + // bad + template: ` + <button :class='style'>Button</button> + ` + + // good + template: ` + <button :class="style">Button</button> + ` + ``` #### Props -1. Props should be declared as an object - ```javascript - // bad - props: ['foo'] - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' +1. Props should be declared as an object + ```javascript + // bad + props: ['foo'] + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } } - } - ``` + ``` 1. Required key should always be provided when declaring a prop - ```javascript - // bad - props: { - foo: { - type: String, - } - } - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' + ```javascript + // bad + props: { + foo: { + type: String, + } } - } - ``` + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } + } + ``` 1. Default key should be provided if the prop is not required. _Note:_ There are some scenarios where we need to check for the existence of the property. On those a default key should not be provided. - ```javascript - // good - props: { - foo: { - type: String, - required: false, - } - } - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' + ```javascript + // good + props: { + foo: { + type: String, + required: false, + } } - } - - // good - props: { - foo: { - type: String, - required: true + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } } - } - ``` + + // good + props: { + foo: { + type: String, + required: true + } + } + ``` #### Data + 1. `data` method should always be a function ```javascript @@ -502,38 +530,41 @@ On those a default key should not be provided. #### Directives 1. Shorthand `@` is preferable over `v-on` - ```javascript - // bad - <component v-on:click="eventHandler"/> - - // good - <component @click="eventHandler"/> - ``` + ```javascript + // bad + <component v-on:click="eventHandler"/> + + // good + <component @click="eventHandler"/> + ``` 1. Shorthand `:` is preferable over `v-bind` - ```javascript - // bad - <component v-bind:class="btn"/> - - // good - <component :class="btn"/> - ``` + ```javascript + // bad + <component v-bind:class="btn"/> + + // good + <component :class="btsn"/> + ``` #### Closing tags + 1. Prefer self closing component tags - ```javascript - // bad - <component></component> - // good - <component /> - ``` + ```javascript + // bad + <component></component> + + // good + <component /> + ``` #### Ordering 1. Tag order in `.vue` file + ``` <script> // ... @@ -550,12 +581,14 @@ On those a default key should not be provided. ``` 1. Properties in a Vue Component: - Check [order of properties in components rule][vue-order]. + Check [order of properties in components rule][vue-order]. #### `:key` + When using `v-for` you need to provide a *unique* `:key` attribute for each item. 1. If the elements of the array being iterated have an unique `id` it is advised to use it: + ```html <div v-for="item in items" @@ -566,6 +599,7 @@ When using `v-for` you need to provide a *unique* `:key` attribute for each item ``` 1. When the elements being iterated don't have a unique id, you can use the array index as the `:key` attribute + ```html <div v-for="(item, index) in items" @@ -575,8 +609,8 @@ When using `v-for` you need to provide a *unique* `:key` attribute for each item </div> ``` - 1. When using `v-for` with `template` and there is more than one child element, the `:key` values must be unique. It's advised to use `kebab-case` namespaces. + ```html <template v-for="(item, index) in items"> <span :key="`span-${index}`"></span> @@ -585,64 +619,69 @@ When using `v-for` you need to provide a *unique* `:key` attribute for each item ``` 1. When dealing with nested `v-for` use the same guidelines as above. - ```html - <div - v-for="item in items" - :key="item.id" - > - <span - v-for="element in array" - :key="element.id" - > - <!-- content --> - </span> - </div> - ``` + ```html + <div + v-for="item in items" + :key="item.id" + > + <span + v-for="element in array" + :key="element.id" + > + <!-- content --> + </span> + </div> + ``` Useful links: + 1. [`key`](https://vuejs.org/v2/guide/list.html#key) 1. [Vue Style Guide: Keyed v-for](https://vuejs.org/v2/style-guide/#Keyed-v-for-essential ) + #### Vue and Bootstrap 1. Tooltips: Do not rely on `has-tooltip` class name for Vue components - ```javascript - // bad - <span - class="has-tooltip" - title="Some tooltip text"> - Text - </span> - // good - <span - v-tooltip - title="Some tooltip text"> - Text - </span> - ``` + ```javascript + // bad + <span + class="has-tooltip" + title="Some tooltip text"> + Text + </span> + + // good + <span + v-tooltip + title="Some tooltip text"> + Text + </span> + ``` 1. Tooltips: When using a tooltip, include the tooltip directive, `./app/assets/javascripts/vue_shared/directives/tooltip.js` 1. Don't change `data-original-title`. - ```javascript - // bad - <span data-original-title="tooltip text">Foo</span> - - // good - <span title="tooltip text">Foo</span> - $('span').tooltip('_fixTitle'); - ``` + ```javascript + // bad + <span data-original-title="tooltip text">Foo</span> + + // good + <span title="tooltip text">Foo</span> + + $('span').tooltip('_fixTitle'); + ``` ### The Javascript/Vue Accord + The goal of this accord is to make sure we are all on the same page. 1. When writing Vue, you may not use jQuery in your application. - 1. If you need to grab data from the DOM, you may query the DOM 1 time while bootstrapping your application to grab data attributes using `dataset`. You can do this without jQuery. - 1. You may use a jQuery dependency in Vue.js following [this example from the docs](https://vuejs.org/v2/examples/select2.html). - 1. If an outside jQuery Event needs to be listen to inside the Vue application, you may use jQuery event listeners. - 1. We will avoid adding new jQuery events when they are not required. Instead of adding new jQuery events take a look at [different methods to do the same task](https://vuejs.org/v2/api/#vm-emit). + 1. If you need to grab data from the DOM, you may query the DOM 1 time while bootstrapping your application to grab data attributes using `dataset`. You can do this without jQuery. + 1. You may use a jQuery dependency in Vue.js following [this example from the docs](https://vuejs.org/v2/examples/select2.html). + 1. If an outside jQuery Event needs to be listen to inside the Vue application, you may use jQuery event listeners. + 1. We will avoid adding new jQuery events when they are not required. Instead of adding new jQuery events take a look at [different methods to do the same task](https://vuejs.org/v2/api/#vm-emit). 1. You may query the `window` object 1 time, while bootstrapping your application for application specific data (e.g. `scrollTo` is ok to access anytime). Do this access during the bootstrapping of your application. 1. You may have a temporary but immediate need to create technical debt by writing code that does not follow our standards, to be refactored later. Maintainers need to be ok with the tech debt in the first place. An issue should be created for that tech debt to evaluate it further and discuss. In the coming months you should fix that tech debt, with it's priority to be determined by maintainers. 1. When creating tech debt you must write the tests for that code before hand and those tests may not be rewritten. e.g. jQuery tests rewritten to Vue tests. @@ -650,6 +689,7 @@ The goal of this accord is to make sure we are all on the same page. 1. Once you have chosen a centralized state management solution you must use it for your entire application. i.e. Don't mix and match your state management solutions. ## SCSS + - [SCSS](style_guide_scss.md) [airbnb-js-style-guide]: https://github.com/airbnb/javascript diff --git a/doc/development/fe_guide/vuex.md b/doc/development/fe_guide/vuex.md index 4089cd37d73..f582f5da323 100644 --- a/doc/development/fe_guide/vuex.md +++ b/doc/development/fe_guide/vuex.md @@ -290,23 +290,24 @@ export default { ``` ### Vuex Gotchas -1. Do not call a mutation directly. Always use an action to commit a mutation. Doing so will keep consistency throughout the application. From Vuex docs: - - > why don't we just call store.commit('action') directly? Well, remember that mutations must be synchronous? Actions aren't. We can perform asynchronous operations inside an action. - ```javascript - // component.vue - - // bad - created() { - this.$store.commit('mutation'); - } +1. Do not call a mutation directly. Always use an action to commit a mutation. Doing so will keep consistency throughout the application. From Vuex docs: - // good - created() { - this.$store.dispatch('action'); - } - ``` + > why don't we just call store.commit('action') directly? Well, remember that mutations must be synchronous? Actions aren't. We can perform asynchronous operations inside an action. + + ```javascript + // component.vue + + // bad + created() { + this.$store.commit('mutation'); + } + + // good + created() { + this.$store.dispatch('action'); + } + ``` 1. Use mutation types instead of hardcoding strings. It will be less error prone. 1. The State will be accessible in all components descending from the use where the store is instantiated. @@ -342,7 +343,7 @@ describe('component', () => { name: 'Foo', age: '30', }; - + store = createStore(); // populate the store diff --git a/doc/development/gotchas.md b/doc/development/gotchas.md index d25d856c3a3..84dea7ce9aa 100644 --- a/doc/development/gotchas.md +++ b/doc/development/gotchas.md @@ -101,8 +101,10 @@ end in a prepended module, which is very likely the case in EE. We could see error like this: - 1.1) Failure/Error: allow_any_instance_of(ApplicationSetting).to receive_messages(messages) - Using `any_instance` to stub a method (elasticsearch_indexing) that has been defined on a prepended module (EE::ApplicationSetting) is not supported. + ``` + 1.1) Failure/Error: allow_any_instance_of(ApplicationSetting).to receive_messages(messages) + Using `any_instance` to stub a method (elasticsearch_indexing) that has been defined on a prepended module (EE::ApplicationSetting) is not supported. + ``` ### Alternative: `expect_next_instance_of` diff --git a/doc/development/i18n/proofreader.md b/doc/development/i18n/proofreader.md index ad5f6b2ecf6..7054ff39da0 100644 --- a/doc/development/i18n/proofreader.md +++ b/doc/development/i18n/proofreader.md @@ -65,7 +65,6 @@ are very appreciative of the work done by translators and proofreaders! Add your language in alphabetical order, and add yourself to the list including: - - name - link to your GitLab profile - link to your CrowdIn profile diff --git a/doc/development/new_fe_guide/development/testing.md b/doc/development/new_fe_guide/development/testing.md index 53dfe6774e9..a9223ac6b0f 100644 --- a/doc/development/new_fe_guide/development/testing.md +++ b/doc/development/new_fe_guide/development/testing.md @@ -6,7 +6,7 @@ * **[Ruby unit tests](#ruby-unit-tests-spec-rb)** for models, controllers, helpers, etc. (`/spec/**/*.rb`) * **[Full feature tests](#full-feature-tests-spec-features-rb)** (`/spec/features/**/*.rb`) * **[Karma](#karma-tests-spec-javascripts-js)** (`/spec/javascripts/**/*.js`) -* ~~Spinach~~ — These have been removed from our codebase in May 2018. (`/features/`) +* <s>Spinach</s> — These have been removed from our codebase in May 2018. (`/features/`) ## RSpec: Ruby unit tests `/spec/**/*.rb` diff --git a/doc/development/new_fe_guide/style/javascript.md b/doc/development/new_fe_guide/style/javascript.md index 57efd9353bc..922fd1e4ea4 100644 --- a/doc/development/new_fe_guide/style/javascript.md +++ b/doc/development/new_fe_guide/style/javascript.md @@ -12,158 +12,157 @@ You can run eslint locally by running `yarn eslint` <a name="avoid-foreach"></a><a name="1.1"></a> - [1.1](#avoid-foreach) **Avoid ForEach when mutating data** Use `map`, `reduce` or `filter` instead of `forEach` when mutating data. This will minimize mutations in functions ([which is aligned with Airbnb's style guide][airbnb-minimize-mutations]) -``` -// bad -users.forEach((user, index) => { - user.id = index; -}); - -// good -const usersWithId = users.map((user, index) => { - return Object.assign({}, user, { id: index }); -}); -``` + ``` + // bad + users.forEach((user, index) => { + user.id = index; + }); + + // good + const usersWithId = users.map((user, index) => { + return Object.assign({}, user, { id: index }); + }); + ``` ## Functions <a name="limit-params"></a><a name="2.1"></a> - [2.1](#limit-params) **Limit number of parameters** If your function or method has more than 3 parameters, use an object as a parameter instead. -``` -// bad -function a(p1, p2, p3) { - // ... -}; - -// good -function a(p) { - // ... -}; -``` + ``` + // bad + function a(p1, p2, p3) { + // ... + }; + + // good + function a(p) { + // ... + }; + ``` ## Classes & constructors <a name="avoid-constructor-side-effects"></a><a name="3.1"></a> - [3.1](#avoid-constructor-side-effects) **Avoid side effects in constructors** Avoid making some operations in the `constructor`, such as asynchronous calls, API requests and DOM manipulations. Prefer moving them into separate functions. This will make tests easier to write and code easier to maintain. - ```javascript - // bad - class myClass { - constructor(config) { - this.config = config; - axios.get(this.config.endpoint) + ```javascript + // bad + class myClass { + constructor(config) { + this.config = config; + axios.get(this.config.endpoint) + } } - } - - // good - class myClass { - constructor(config) { - this.config = config; + + // good + class myClass { + constructor(config) { + this.config = config; + } + + makeRequest() { + axios.get(this.config.endpoint) + } } - - makeRequest() { - axios.get(this.config.endpoint) - } - } - const instance = new myClass(); - instance.makeRequest(); - - ``` + const instance = new myClass(); + instance.makeRequest(); + + ``` <a name="avoid-classes-to-handle-dom-events"></a><a name="3.2"></a> - [3.2](#avoid-classes-to-handle-dom-events) **Avoid classes to handle DOM events** If the only purpose of the class is to bind a DOM event and handle the callback, prefer using a function. -``` -// bad -class myClass { - constructor(config) { - this.config = config; - } - - init() { - document.addEventListener('click', () => {}); - } -} - -// good - -const myFunction = () => { - document.addEventListener('click', () => { - // handle callback here - }); -} -``` + ``` + // bad + class myClass { + constructor(config) { + this.config = config; + } + + init() { + document.addEventListener('click', () => {}); + } + } + + // good + + const myFunction = () => { + document.addEventListener('click', () => { + // handle callback here + }); + } + ``` <a name="element-container"></a><a name="3.3"></a> - [3.3](#element-container) **Pass element container to constructor** When your class manipulates the DOM, receive the element container as a parameter. This is more maintainable and performant. -``` -// bad -class a { - constructor() { - document.querySelector('.b'); - } -} - -// good -class a { - constructor(options) { - options.container.querySelector('.b'); - } -} -``` + ``` + // bad + class a { + constructor() { + document.querySelector('.b'); + } + } + + // good + class a { + constructor(options) { + options.container.querySelector('.b'); + } + } + ``` ## Type Casting & Coercion <a name="use-parseint"></a><a name="4.1"></a> - [4.1](#use-parseint) **Use ParseInt** Use `ParseInt` when converting a numeric string into a number. -``` -// bad -Number('10') - - -// good -parseInt('10', 10); -``` + ``` + // bad + Number('10') + + // good + parseInt('10', 10); + ``` ## CSS Selectors <a name="use-js-prefix"></a><a name="5.1"></a> - [5.1](#use-js-prefix) **Use js prefix** If a CSS class is only being used in JavaScript as a reference to the element, prefix the class name with `js-` -``` -// bad -<button class="add-user"></button> - -// good -<button class="js-add-user"></button> -``` + ``` + // bad + <button class="add-user"></button> + + // good + <button class="js-add-user"></button> + ``` ## Modules <a name="use-absolute-paths"></a><a name="6.1"></a> - [6.1](#use-absolute-paths) **Use absolute paths for nearby modules** Use absolute paths if the module you are importing is less than two levels up. -``` -// bad -import GitLabStyleGuide from '~/guides/GitLabStyleGuide'; - -// good -import GitLabStyleGuide from '../GitLabStyleGuide'; -``` + ``` + // bad + import GitLabStyleGuide from '~/guides/GitLabStyleGuide'; + + // good + import GitLabStyleGuide from '../GitLabStyleGuide'; + ``` <a name="use-relative-paths"></a><a name="6.2"></a> - [6.2](#use-relative-paths) **Use relative paths for distant modules** If the module you are importing is two or more levels up, use a relative path instead of an absolute path. -``` -// bad -import GitLabStyleGuide from '../../../guides/GitLabStyleGuide'; - -// good -import GitLabStyleGuide from '~/GitLabStyleGuide'; -``` + ``` + // bad + import GitLabStyleGuide from '../../../guides/GitLabStyleGuide'; + + // good + import GitLabStyleGuide from '~/GitLabStyleGuide'; + ``` <a name="global-namespace"></a><a name="6.3"></a> - [6.3](#global-namespace) **Do not add to global namespace** diff --git a/doc/development/newlines_styleguide.md b/doc/development/newlines_styleguide.md index 32aac2529a4..5f7210020b6 100644 --- a/doc/development/newlines_styleguide.md +++ b/doc/development/newlines_styleguide.md @@ -10,7 +10,7 @@ def method issue = Issue.new issue.save - + render json: issue end ``` @@ -20,7 +20,7 @@ end def method issue = Issue.new issue.save - + render json: issue end ``` diff --git a/doc/development/sidekiq_debugging.md b/doc/development/sidekiq_debugging.md index d6d770e27c1..84b61bd7e61 100644 --- a/doc/development/sidekiq_debugging.md +++ b/doc/development/sidekiq_debugging.md @@ -3,8 +3,7 @@ ## Log arguments to Sidekiq jobs If you want to see what arguments are being passed to Sidekiq jobs you can set -the `SIDEKIQ_LOG_ARGUMENTS` [environment variable] -(https://docs.gitlab.com/omnibus/settings/environment-variables.html) to `1` (true). +the `SIDEKIQ_LOG_ARGUMENTS` [environment variable](https://docs.gitlab.com/omnibus/settings/environment-variables.html) to `1` (true). Example: diff --git a/doc/development/testing_guide/ci.md b/doc/development/testing_guide/ci.md index 0d8e150e090..8d9706a9501 100644 --- a/doc/development/testing_guide/ci.md +++ b/doc/development/testing_guide/ci.md @@ -5,23 +5,23 @@ Our current CI parallelization setup is as follows: 1. The `knapsack` job in the prepare stage that is supposed to ensure we have a - `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file: - - The `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file is fetched - from S3, if it's not here we initialize the file with `{}`. + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file: + - The `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file is fetched + from S3, if it's not here we initialize the file with `{}`. 1. Each `rspec x y` job are run with `knapsack rspec` and should have an evenly - distributed share of tests: - - It works because the jobs have access to the - `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` since the "artifacts - from all previous stages are passed by default". [^1] - - the jobs set their own report path to - `KNAPSACK_REPORT_PATH=knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json`. - - if knapsack is doing its job, test files that are run should be listed under - `Report specs`, not under `Leftover specs`. + distributed share of tests: + - It works because the jobs have access to the + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` since the "artifacts + from all previous stages are passed by default". + - the jobs set their own report path to + `KNAPSACK_REPORT_PATH=knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json`. + - if knapsack is doing its job, test files that are run should be listed under + `Report specs`, not under `Leftover specs`. 1. The `update-knapsack` job takes all the - `knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json` - files from the `rspec x y` jobs and merge them all together into a single - `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file that is then - uploaded to S3. + `knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json` + files from the `rspec x y` jobs and merge them all together into a single + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file that is then + uploaded to S3. After that, the next pipeline will use the up-to-date `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file. diff --git a/doc/development/testing_guide/testing_levels.md b/doc/development/testing_guide/testing_levels.md index 4403072e96f..32ed22ca3ed 100644 --- a/doc/development/testing_guide/testing_levels.md +++ b/doc/development/testing_guide/testing_levels.md @@ -153,7 +153,7 @@ trade-off: - Unit tests are usually cheap, and you should consider them like the basement of your house: you need them to be confident that your code is behaving correctly. However if you run only unit tests without integration / system - tests, you might [miss] the [big] [picture]! + tests, you might [miss] the [big] / [picture] ! - Integration tests are a bit more expensive, but don't abuse them. A system test is often better than an integration test that is stubbing a lot of internals. - System tests are expensive (compared to unit tests), even more if they require |