Linting consistency

I swear I’ve copied all the lint stuff from discourse-plugin-skeleton to my plugin and when I run

pnpm prettier  --write "assets/**/*.{scss,js,gjs,hbs}"

in my plugin directory, I get something different than when I run

./bin/lint  --fix plugins/discourse-pfaffmanager/assets/**/*.{scss,js,gjs,hbs}

in the discourse core directory.

Predictably, I guess, the latter is the one that matches what happens when github actions run.

What I’d really like is for the Right Thing to happen when I save in VSCode, which, I’m pretty sure, it yet another issue.

3 likes

Yeah @cvx / @david

I also noticed this… linting in plugins is all snowflake material, we have branches for old vs new style of linting in CI and require a bunch of setup in the repo.

I think a great goal is to fix it so bin/lint works for plugins and themes as well. We have not implemented that yet, it only works for core and core plugins.

2 likes

Indeed, linting for themes/plugins is deliberately decoupled from core. They might run different versions & configs of the linting tools. This is essential so that we can roll out changes without suddenly breaking linting in all repos.

We could probably make bin/lint work - it’ll need to cd into the plugin/theme directory, pnpm i, and then run any linting commands in the context of that directory. But it must not attempt to use core’s linting config on themes/plugins.

It should. Most of our team use VSCode (or VSCode derivations), so that’s what we optimize for. Eslint and Prettier VSCode plugins will search for the ‘nearest’ installation & config for a given file.

Have you run pnpm i in the theme/plugin directory? That’s required to install its linting dependencies, so that VSCode can find the right copy of prettier/eslint to use.

1 like

Interesting twists

  • Allow it to optionally run cores rules
  • Allow it to reconfigure existing config to run cores rules

What would be the benefit of that? :sweat_smile:

Theme/plugin linting is fundamentally different to core. We have theme/plugin specific rules. Themes have their own magic globals which ESLint needs to know about (themePrefix/settings)

We’re very careful about maintaining this, documenting it in the theme/plugin-skeleton repos, and rolling out changes methodically via mass-pr.

2 likes

As a tool for self upgrading, but I totally get we have other systems

Ah ok. We have scripts for automatic updating (from the skeleton) here. Making those more friendly to run on single themes/plugins could be a nice project. :+1:

BTW, when I say they have different config, I don’t mean we’re duplicating the config everywhere. We have a versioned lint configs package which includes shared configs for prettier, eslint, stylelint, and ember-template-lint. This is used by core and all themes/plugins. Upgrading a theme/plugin to the latest rules is normally just a case of bumping the lint-configs version in the package.json.

1 like

But I’m pretty sure that when I follow the linting done in the plugin, the linting tests fail in the github action.

AHA!!! pnpm i seems to be the magic I’m missing. I’ve added the ‘pnpm i’ to my script that updates discourse core and the one that does the linting. . .

So now here’s what my script does:

if [[ "$ARG" == "eslint" ]]
then
  # See https://github.com/discourse/.github/blob/main/.github/workflows/discourse-plugin.yml
  if [ -f plugin.rb ]
  then
    echo "Linting current directory."
  else
    echo "Not on a plugin. Using $DEFAULT_PLUGIN"
    cd $DEFAULT_PLUGIN
  fi
  echo "LINTING $(pwd)"
  #cd /home/pfaffman/src/literatecomputing/discourse-display-email
  # pnpm install
  echo "Doing `pnpm i`"
  pnpm -i

  echo -n ESLINT. . .
  pnpm eslint --no-error-on-unmatched-pattern {test,assets,admin/assets}/javascripts
  exit_code="$?"
  if [[ $exit_code -ne 0  ]]
  then
    echo "eslint was unhappy -- $exit_code -- trying to fix"
    pnpm eslint --fix --no-error-on-unmatched-pattern {test,assets,admin/assets}/javascripts
    exit_code="$?"
    if [[ $exit_code -ne 0  ]]
    then
      echo "eslint was still unhappy -- $exit_code -- game over"
      exit
    fi
  fi

  echo ESLINT done.

  ## styllint

  echo -n STYLELINT . . .
  pnpm stylelint --allow-empty-input "assets/**/*.scss"
  exit_code="$?"
    if [[ $exit_code -ne 0  ]]
    then
      echo "stylelint is unhappy. Trying to fix . . . "
    fi
  pnpm stylelint --fix --allow-empty-input "assets/**/*.scss"
  pnpm stylelint --allow-empty-input "assets/**/*.scss"
  exit_code="$?"
    if [[ $exit_code -ne 0  ]]
    then
      echo "stylelint is still unhappy. this is sad. Game over."
      exit
    fi
  echo "DONE!"

  ## end stylelint

  ## PRETTIER

  echo -n "Prettier . . . "
  if [ -n "$(find assets -type f \( -name "*.scss" -or -name "*.js" -or -name "*.gjs" -or -name "*.hbs" \) 2>/dev/null)" ]; then
    #pnpm prettier --write                  'assets/**/*.{scss,js,gjs,es6,hbs}'
    # echo "doing pnpm prettier --write --log-level warn assets/**/*.{scss,js,gjs,es6,hbs}"
    pnpm prettier --write --log-level error "assets/**/*.{scss,js,gjs,hbs}"
    if [[ $? -ne 0  ]]
    then
      echo "prettier assets did something!!! -- $?"
      sleep 5
    fi
  fi
  if [ -n "$(find admin/assets -type f \( -name "*.scss" -or -name "*.js" -or -name "*.gjs" -or -name "*.hbs" \) 2>/dev/null)" ]; then
    #pnpm prettier --write                  'assets/**/*.{scss,js,gjs,es6,hbs}'
    # echo "doing pnpm prettier --write --log-level warn admin/assets/**/*.{scss,js,gjs,hbs}"
    pnpm prettier --write --log-level log "assets/**/*.{scss,js,gjs,es6,hbs}"
    if [[ $? -ne 0  ]]
    then
      echo "prettier admin/assets did something!!! -- $?"
      pnpm prettier --check --log-level log "assets/**/*.{scss,js,gjs,es6,hbs}"
    fi
  fi
  if [ -n "$(find test -type f \( -name "*.js" -or -name "*.gjs" \) 2>/dev/null)" ]; then
    #pnpm prettier --write                  'assets/**/*.{scss,js,gjs,es6,hbs}'
    # echo "doing pnpm prettier --write --log-level warn assets/**/*.{scss,js,gjs,es6,hbs}"
    pnpm prettier --write --log-level warn "test/**/*.{js,gjs}"
    if [[ $? -ne 0  ]]
    then
      echo "prettier test did something!!! -- $?"
      pnpm prettier --check --loglevel log "test/**/*.{js,gjs}"
    fi
  fi
  echo "Done with prettier"

  echo "ember-template-lint"
  pnpm ember-template-lint --fix --no-error-on-unmatched-pattern assets/javascripts admin/assets/javascripts
  exit_code="$?"
    if [[ $exit_code -ne 0  ]]
    then
      echo "done with ember-template-lint --fix --no-error-on-unmatched-pattern assets/javascripts -- with $exit_code"
      echo sleep 5
      sleep 5
    fi
  #bundle exec stree write Gemfile $(git ls-files '*.rb') $(git ls-files '*.rake') $(git ls-files '*.thor')
  bundle exec stree check Gemfile $(git ls-files '*.rb') $(git ls-files '*.rake') $(git ls-files '*.thor')
  exit_code="$?"
    if [[ $exit_code -ne 0  ]]
    then
      echo "stree is unhappy. Trying to fix in 2 seconds . .. "
      sleep 2
      echo "here we go!!!"
      bundle exec stree write Gemfile $(git ls-files '*.rb') $(git ls-files '*.rake') $(git ls-files '*.thor')
    fi
  bundle exec stree check Gemfile $(git ls-files '*.rb') $(git ls-files '*.rake') $(git ls-files '*.thor')
  exit_code="$?"
    if [[ $exit_code -ne 0  ]]
    then
      echo "stree is still unhappy. this is sad. Waiting to make you sadder"
      sleep 15
    fi
  echo done with stree

  echo "rubocop!"
  bundle exec rubocop -A $(find . -name "*.rb"|grep -v gems)
  exit_code="$?"
  if [[ $exit_code -ne 0  ]]
  then
    echo "done with rubocop -- $exit_code"
    sleep 15
  fi
  exit
fi


But I also have this, which I think works almost the same:

if [[ "$ARG" ==  "lint" ]]
then
  cd ~/src/discourse-repos/discourse
  find plugins/discourse-pfaffmanager/assets -type f \( -name "*.scss" -o -name "*.js" -o -name "*.gjs" -o -name "*.hbs" -o -name "*.rb" \) -exec ./bin/lint --fix {} +
  cd -
fi

The only difference now seems to be that core linting is unhappy about this (and I think I understand why core finds it and plugin doesn’t, and there’s no downside to fixing up those deprecations sooner rather than later):


/home/pfaffman/src/discourse-repos/discourse/plugins/discourse-pfaffmanager/assets/javascripts/discourse/components/modal/really-delete.gjs
  2:8  error  Use Glimmer components(@glimmer/component) instead of classic components(@ember/component)  ember/no-classic-components

✖ 1 problem (1 error, 0 warnings)

And if I use bin/lint then I don’t have to worry that something will change in linting that requires updating my 54 line script. Right?

Wait. Documenting them where in the skeleton repos?

I’d love some guidance on how I might roll out changes methodically, though I understand that my tiny set of stuff isn’t really your problem. . . This doesn’t really belong here, but I’ve tried a couple of times to figure out mass-pr, but haven’t yet succeeded. I usually just periodically copy some .whatever files from a recent pull of the skeleton and hope for the best.

And there are 16 of those scripts. Do I watch the repo and then run it anytime any of them is updated? And a bunch of theme seem not to clone the skeletons so they fail to copy the files required?

And several seem to fail with something like sed: can’t read s/default.yml/stree-compat.yml/: No such file or directory ? because they don’t clone the skeleton repos? (I’ll start a topic somewhere appropriate for that when I get a chance).