Clean `dlint` flags #15290

akrmn · 2022-10-19T15:12:47Z

This cleans up a few issues I found when looking at the lint-related options. The main ones are:

damlc commands that don't care about lint options no longer accept any lint options.
It is now possible to specify the "base" rules file by its full path (rather than the path of its containing directory with the file being forced to have the name dlint.yaml).
It is now possible to specify multiple concrete hints files ("overrides") rather than just having the option of the implicit .dlint.yaml from the closest ancestor or the user's home directory.
damlc lint no longer accepts options to enable/disable linting.
Parsed values of optDlintUsage are no longer overridden by the compiler anywhere.

changelog_begin changelog_end

akrmn · 2022-10-20T12:44:26Z

compiler/damlc/daml-ide-core/src/Development/IDE/Core/Rules/Daml.hs

-      else
-        return Nothing
+dlintSettings :: DlintUsage -> IO ([Classify], Hint)
+dlintSettings DlintDisabled = pure ([], mempty @Hint)


dlintSettings now works with DlintUsage instead of just the fields in DlintEnabled, so it can return the empty settings for DlintDisabled

Naive question. Do we need the @Hint here ?

akrmn · 2022-10-20T12:46:22Z

compiler/damlc/daml-ide-core/src/Development/IDE/Core/Rules/Daml.hs

    defineNoFile $ \GetDlintSettings ->
-      liftIO $ case usage of
-          DlintEnabled dir enableOverrides -> dlintSettings dir enableOverrides
-          DlintDisabled -> fail "linter configuration unspecified"
+      liftIO $ dlintSettings usage


now GetDlintSettings can't fail

akrmn · 2022-10-20T12:52:29Z

compiler/damlc/daml-ide-core/src/Development/IDE/Core/Rules/Daml.hs

+      getDlintRulesFile :: DlintRulesFile -> IO FilePath
+      getDlintRulesFile = \case
+        DefaultDlintRulesFile -> locateRunfiles $
+          mainWorkspace </> "compiler" </> "damlc" </> "daml-ide-core" </> "dlint.yaml"
+        ExplicitDlintRulesFile path -> pure path


Previously dlintSettings simply got dlintDataDir :: FilePath as an argument, so all callers had to set the value to something, 99% of the time to the result of locateRunfiles $ mainWorkspace </> "compiler" </> "damlc" </> "daml-ide-core". Now, dlintSettings does a single lookup instead when dlintRulesFile = DefaultDlintRulesFile.

akrmn · 2022-10-20T12:54:02Z

compiler/damlc/daml-ide-core/src/Development/IDE/Core/Rules/Daml.hs

-      (dlintDataDir </> "dlint.yaml") : maybeToList dlintYaml
+      dlintRulesFile : hintFiles


Since the only reason for dlintDataDir was to find the dlint.yaml file directly inside it, now we just talk about the file directly. This has the benefit of allowing users to specify any file as the rules file instead of having to name it dlint.yaml and specify the directory

akrmn · 2022-10-20T12:55:08Z

compiler/damlc/daml-ide-core/test/Development/IDE/Core/API/Testing.hs

@@ -152,9 +151,9 @@ runShakeTest = runShakeTestOpts id
 -- | Run shake test on freshly initialised shake service, with custom options.
 runShakeTestOpts :: (Daml.Options -> Daml.Options) -> Maybe SS.Handle -> ShakeTest () -> IO (Either ShakeTestError ShakeTestResults)
 runShakeTestOpts fOpts mbScenarioService (ShakeTest m) = do
-    dlintDataDir <-locateRunfiles $ mainWorkspace </> "compiler/damlc/daml-ide-core"


we no longer need to locateRunfiles here

akrmn · 2022-10-20T13:18:34Z

compiler/damlc/lib/DA/Cli/Damlc.hs

-     setDlintDataDir :: Options -> IO Options
-     setDlintDataDir opts = do
-       defaultDir <-locateRunfiles $
-         mainWorkspace </> "compiler/damlc/daml-ide-core"
-       return $ case optDlintUsage opts of
-         DlintEnabled _ _ -> opts
-         DlintDisabled  -> opts{optDlintUsage=DlintEnabled defaultDir True}


now that cmdLint only allows the user to change the configuration of the linter, but not to turn it off, we don't need to hardcode these

akrmn · 2022-10-20T13:19:42Z

compiler/damlc/lib/DA/Cli/Damlc.hs

@@ -726,8 +762,7 @@ execRepl dars importPkgs mbLedgerConfig mbAuthToken mbAppId mbSslConf mbMaxInbou
            -- We change directory so make this absolute
            dars <- mapM makeAbsolute dars
            opts <- pure opts
-                { optDlintUsage = DlintDisabled


now that cmdRepl forbids any linter flags, we don't need to hardcode it off here.

akrmn · 2022-10-20T13:22:47Z

compiler/damlc/lib/DA/Cli/Damlc/Command/Damldoc.hs

+          numProcessors
+          (EnableScenarioService False)
+          optPackageName
+          disabledDlintUsageParser


no linter flags for daml damlc docs

akrmn · 2022-10-20T13:28:48Z

compiler/damlc/lib/DA/Cli/Options.hs

+  fromParsed
+    <$> lastOr def (enableDlint <|> disableDlint)
+    <*> dlintOptionsParser


Parsing the on/off flags separately from the options keeps the parser simple

akrmn · 2022-10-20T13:29:46Z

compiler/damlc/tests/src/DA/Test/DamlcIntegration.hs

@@ -196,8 +196,6 @@ getIntegrationTests registerTODO scenarioService = do
    let outdir = "compiler/damlc/output"
    createDirectoryIfMissing True outdir

-    dlintDataDir <- locateRunfiles $ mainWorkspace </> "compiler/damlc/daml-ide-core"


Another place where we no longer need to locateRunfiles

akrmn · 2022-10-20T13:42:13Z

compiler/damlc/lib/DA/Cli/Options.hs

+    clearDlintHintFiles =
+      flag' NoDlintHintFiles
+        ( long "lint-no-hint-files"
+          <> internal
+          <> help "Use no hint files for linting. This also ignores any \
+                  \implicit '.dlint.yaml' files"
+        )


Since --lint-hint-file above allows multiple files to be specified, it's useful to have a way to clear them

remyhaemmerle-da

LGTM. Thanks.

akrmn changed the title ~~wip~~ [WIP] Clean dlint flags Oct 19, 2022

akrmn force-pushed the clean-lint-flags branch 4 times, most recently from 590362a to e099263 Compare October 20, 2022 12:38

Clean dlint flags

d0a074f

changelog_begin changelog_end

akrmn force-pushed the clean-lint-flags branch from e099263 to d0a074f Compare October 20, 2022 13:01

akrmn added 3 commits October 20, 2022 15:05

ofInterestRule no longer needs Options

956e288

Inline and remove 'defaultDlintOptions'

f7273ee

Add and use pattern NoDlintHintFiles = ExplicitDlintHintFiles []

80d78a8

akrmn commented Oct 20, 2022

View reviewed changes

akrmn changed the title ~~[WIP] Clean dlint flags~~ Clean dlint flags Oct 20, 2022

akrmn marked this pull request as ready for review October 20, 2022 14:27

akrmn requested review from basvangijzel-DA, remyhaemmerle-da and dylant-da as code owners October 20, 2022 14:27

akrmn requested review from a team October 20, 2022 14:27

remyhaemmerle-da approved these changes Oct 24, 2022

View reviewed changes

akrmn merged commit 0dbc042 into main Oct 25, 2022

akrmn deleted the clean-lint-flags branch October 25, 2022 14:12

akrmn mentioned this pull request Nov 4, 2022

Clean up uses of locateRunfiles #15446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean `dlint` flags #15290

Clean `dlint` flags #15290

akrmn commented Oct 19, 2022 •

edited

Loading

akrmn Oct 20, 2022

remyhaemmerle-da Oct 21, 2022

akrmn Oct 20, 2022

akrmn Oct 20, 2022

akrmn Oct 20, 2022

akrmn Oct 20, 2022

akrmn Oct 20, 2022

akrmn Oct 20, 2022

akrmn Oct 20, 2022

akrmn Oct 20, 2022

akrmn Oct 20, 2022

akrmn Oct 20, 2022

remyhaemmerle-da left a comment

		(dlintDataDir </> "dlint.yaml") : maybeToList dlintYaml
		dlintRulesFile : hintFiles

Clean dlint flags #15290

Clean dlint flags #15290

Conversation

akrmn commented Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remyhaemmerle-da left a comment

Choose a reason for hiding this comment

Clean `dlint` flags #15290

Clean `dlint` flags #15290

akrmn commented Oct 19, 2022 •

edited

Loading