Alignment of <-s is really aggressive #21

New Issue

2017-04-09T22:58:57+02:00

barrucadu commented

2017-04-09 22:58:57 +02:00

(Migrated from github.com)

When aligning things by hand, I usually decide not to if it would introduce "too much" spacing (where "too much" is some fuzzy thing based on gut feeling).

This is definitely too much:

   (finalCtx, trace, finalAction) <- runThreads sched memtype ref ctx
-  out <- readRef ref
+  out                            <- readRef ref

Perhaps there could be a configurable maximum number of spaces, with alignment not done if it would introduce more than that many?

When aligning things by hand, I usually decide not to if it would introduce "too much" spacing (where "too much" is some fuzzy thing based on gut feeling). This is definitely too much: ```diff (finalCtx, trace, finalAction) <- runThreads sched memtype ref ctx - out <- readRef ref + out <- readRef ref ``` Perhaps there could be a configurable maximum number of spaces, with alignment not done if it would introduce more than that many?

lspitzner commented

2017-04-10 00:23:21 +02:00

(Migrated from github.com)

good idea. There is also "don't do alignment when the things being aligned are apart more than x lines", which is like the vertical version of this.

barrucadu commented

2017-04-11 18:42:27 +02:00

(Migrated from github.com)

For the vertical version, I think it would be worth distinguishing lines of comments/whitespace, and lines of code.

For example, I align the ::s here because there are only a few lines of comments between them:

-- | DPOR execution is represented as a tree of states, characterised
-- by the decisions that lead to that state.
data DPOR = DPOR
  { dporRunnable :: Set ThreadId
  -- ^ What threads are runnable at this step.
  , dporTodo     :: Map ThreadId Bool
  -- ^ Follow-on decisions still to make, and whether that decision
  -- was added conservatively due to the bound.
  , dporDone     :: Map ThreadId DPOR
  -- ^ Follow-on decisions that have been made.
  , dporSleep    :: Map ThreadId ThreadAction
  -- ^ Transitions to ignore (in this node and children) until a
  -- dependent transition happens.
  , dporTaken    :: Map ThreadId ThreadAction
  -- ^ Transitions which have been taken, excluding
  -- conservatively-added ones. This is used in implementing sleep
  -- sets.
  , dporAction   :: Maybe ThreadAction
  -- ^ What happened at this step. This will be 'Nothing' at the root,
  -- 'Just' everywhere else.
  } deriving (Eq, Show)

But I don't align the ->s in the case here, because there is code in the way:

       case M.lookup tid' (dporDone dpor) of
         Just dpor' ->
           let done = M.insert tid' (grow state' tid' rest dpor') (dporDone dpor)
           in dpor { dporDone = done }
         Nothing ->
           let taken = M.insert tid' a (dporTaken dpor)
               sleep = dporSleep dpor `M.union` dporTaken dpor
               done  = M.insert tid' (subtree state' tid' sleep trc) (dporDone dpor)
           in dpor { dporTaken = if conservative then dporTaken dpor else taken
                   , dporTodo  = M.delete tid' (dporTodo dpor)
                   , dporDone  = done
                   }

For the vertical version, I think it would be worth distinguishing lines of comments/whitespace, and lines of code. For example, I align the `::`s here because there are only a few lines of comments between them: ```haskell -- | DPOR execution is represented as a tree of states, characterised -- by the decisions that lead to that state. data DPOR = DPOR { dporRunnable :: Set ThreadId -- ^ What threads are runnable at this step. , dporTodo :: Map ThreadId Bool -- ^ Follow-on decisions still to make, and whether that decision -- was added conservatively due to the bound. , dporDone :: Map ThreadId DPOR -- ^ Follow-on decisions that have been made. , dporSleep :: Map ThreadId ThreadAction -- ^ Transitions to ignore (in this node and children) until a -- dependent transition happens. , dporTaken :: Map ThreadId ThreadAction -- ^ Transitions which have been taken, excluding -- conservatively-added ones. This is used in implementing sleep -- sets. , dporAction :: Maybe ThreadAction -- ^ What happened at this step. This will be 'Nothing' at the root, -- 'Just' everywhere else. } deriving (Eq, Show) ``` But I don't align the `->`s in the case here, because there is code in the way: ```haskell case M.lookup tid' (dporDone dpor) of Just dpor' -> let done = M.insert tid' (grow state' tid' rest dpor') (dporDone dpor) in dpor { dporDone = done } Nothing -> let taken = M.insert tid' a (dporTaken dpor) sleep = dporSleep dpor `M.union` dporTaken dpor done = M.insert tid' (subtree state' tid' sleep trc) (dporDone dpor) in dpor { dporTaken = if conservative then dporTaken dpor else taken , dporTodo = M.delete tid' (dporTodo dpor) , dporDone = done } ```

lspitzner commented

2017-04-11 18:45:03 +02:00

(Migrated from github.com)

yes, in fact i think not ignoring comments for this test would be much harder than what you propose, given how things are implemented :)

yes, in fact i think _not_ ignoring comments for this test would be much harder than what you propose, given how things are implemented :)

lspitzner commented

2017-05-17 23:36:25 +02:00

(Migrated from github.com)

, _lconfig_alignmentLimit = coerce (30 :: Int)
, _lconfig_alignmentBreakOnMultiline = coerce True

i hope these defaults are fine. please report if this doesn't work as expected.

> , _lconfig_alignmentLimit = coerce (30 :: Int) > , _lconfig_alignmentBreakOnMultiline = coerce True i hope these defaults are fine. please report if this doesn't work as expected.

Sign in to join this conversation.

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#21