8000 PSA: The large reformat · Issue #240 · nrkno/sofie-core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

PSA: The large reformat #240

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.

Sign up for GitHub

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

Jump to bottom
Closed
Julusian opened this issue May 29, 2020 · 3 comments
Closed

PSA: The large reformat #240

Julusian opened this issue May 29, 2020 · 3 comments
Labels
📢 PSA An announcement about upcoming changes

Comments

@Julusian
8000 Copy link
Contributor
Julusian commented May 29, 2020 •
edited
Loading

As we planned to do many months ago, this repository has now been reformatted using prettier.
This can cause large and painful merge conflicts in pull requests unless handled carefully. This issue aims to outline advice and steps to mitigate any pain that may be felt because of this.

Process for a painless merge

The best process for handling this, is to merge the formatted branch into yours.

  1. Firstly, you need to identify the 2 important commits from the branch you want to merge in. These are the commit just before the merge, and the commit of the merge. To identify these, look in the git history for the merge commit adding the prettier config, and a merge commit soon after which applies this formatting. Github does not make the correct commits very clear, some desktop tools show it a lot better.
    For the release21 branch, these are 4864ff5 and 6314c74.
    For the master branch, these are d7a403c and 16cf61d.
    Note: the remaining steps will assume you are working with master, and will use the commit ids as shown above

  2. checkout your branch to update

  3. Pull in any changes git merge d7a403c6
    IMPORTANT: any merge conflicts need to be committed with git commit --no-verify to avoid premature formatting of the changes

  4. If you are on windows, you will need to cherry-pick in 761f927 and d36db2a to get the scripts to work properly

  5. Pull in the format change git merge 16cf61da

  6. Discard any changes, and delete any 'new' files

  7. meteor npm run quickformat

  8. meteor npm run lint -- --fix
    Note: Ensure no errors are reported by this. any which are need to be fixed before continuing

  9. Commit all changed files. This should be done as a normal commit again, and will verify the lint is ok

  10. Pull in any newer changes git merge master
@Julusian Julusian added the 📢 PSA An announcement about upcoming changes label May 29, 2020
@Julusian Julusian mentioned this issue May 29, 2020
Closed
@jstarpl
Copy link
Member
jstarpl commented Jun 1, 2020 •
edited
Loading

There seems to be some problems developing on Windows now, after the reformat/workflow change:

  1. meteor npm run quickformat works only on Unix-based systems (the command syntax used in the script is Unix-specific)
  2. the husky may work only on Unix-based systems (the way filenames are piped to the linter may cause an "ENAMETOOLONG" exception, if the number of staged files is significant), there is a suggestion to look up: https://github.com/okonet/lint-staged#using-js-functions-to-customize-tasks which suggest just running the commands on an entire repository if the resulted argument list is expected to be too long (https://github.com/okonet/lint-staged#example-run-eslint-on-entire-repo-if-more-than-10-staged-files).

I'll be looking into fixing these issues, but I just thought I would let anyone else having these issues know. Took me a while to figure out what was wrong.

@Julusian
Copy link
Contributor Author
Julusian commented Jun 2, 2020 •
edited
Loading
  1. Oops, hopefully an easy fix? That is intended to be a temporary command, so feel free to make even more of a mess of it, or perhaps duplicate and make a windows specific version
  2. I did digging, and it appears to have been fixed in lint-staged v10 (which requires node 10) feat: split tasks into chunks to support shells with limited max argument length lint-staged/lint-staged#732 The command line is too long(windows) lint-staged/lint-staged#147. But was also supported for a period up until v9, so the quick fix might be to downgrade lint-staged to v8.2.1 until we get meteor 1.10

@jstarpl
Copy link
Member
jstarpl commented Jun 2, 2020

I think I was able to fix both issues here: #242

@nytamin nytamin closed this as completed Sep 23, 2020
ianshade pushed a commit to ianshade/tv-automation-server-core that referenced this issue Jun 9, 2023
@LindvedKrvang
Merge pull request nrkno#240 from tv2/SOF-1417/listOfSchemaHasDuplicates
24b38ed 
Sof 1417/list of schema has duplicates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
📢 PSA An announcement about upcoming changes
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Julusian @jstarpl @nytamin
0