customizable edge pattern #93.

[kukimik]

Closes #93. (I've noticed this is a duplicate of #33.)

I included the new option in the same manner as the existing ones. This means the field is mandatory in the .yaml config file. However, this implies that the change is breaking for people who use the config file.

Perhaps it would be better to make the edge-pattern field optional in the .yaml config? If so, what about dot-entity (which is not in the latest 0.2.1.0-RC1 release) and other options? I believe they should be optional too, but I'd rather know your opinion before I make the change.

[mmzx]

Thank you for your contribution!

I would definitely vote for the optional way of doing things - in this case -, that is, when the property is not defined it shall behave as a fallback to a default. In the morning (GMT+1) I will take a better look at your work.

[kukimik]

I would definitely vote for the optional way of doing things

Ok, I'll try to make this optional next week.

[mmzx]

FYI, the CI result may be followed here. It appears "external" CI services are not supported on the github UI any more.

[kukimik]

Thanks for the info. Today github shows me a link to the CI results.

I made all entries in the config file optional. However, at present, an empty config file is still trreated as if it didn't exist. I'll look into it.

[kukimik]

Using an empty config file (or a file consisting only of comments) will now result in setting all the options to default values. The PR is ready to be reviewed.

The whole configuration mechanism could, in my opinion, be improved, but - if it's needed - I'd rather do it in a separate PR.

[kukimik]

@mmzx I have a PR ready to be made that would close #92 (and the related issue #33). It's at https://github.com/kukimik/erd/tree/92_crow_foot. Would you rather have a separate PR for this, or should I combine it with this PR? The changes in both cases are very similar, so maybe it would be easier for you to review it together?

[mmzx]

@kukimik , I separate PR would be nice to have. I went through the code and tried the added changes, it works. :) It would be nice to improve on the solution that uses fromJust. - by not using fromJust itself.

The configuration file shall be a separate matter.

[kukimik]

@mmzx Thanks for checking the code and your comments.

separate PR would be nice to have.

Ok, I'll create it once this is merged.

It would be nice to improve on the solution that uses fromJust. - by not using fromJust itself.

Do you mean unsafeFromConfigOrDefault in app/Main.hs?

The solution using fromJust was there before my PR:

             , A.Splines $ fromMaybe (fromJust . edgeType $ defaultConfig) (edgeType conf)

I only extracted this into a function and gave it a name.

This is one of the things that I had in mind when I said: "The whole configuration mechanism could, in my opinion, be improved, but - if it's needed - I'd rather do it in a separate PR." To repair this I'd have to change the type of Erd.Config.defaultConfig to get rid of Maybe from the default configuration. For example I can parametrize the type Erd.Config.Config:

data Config f = Config
    { cin         :: (String, Handle)
    , cout        :: (String, Handle)
    , outfmt      :: f G.GraphvizOutput
    , edgeType    :: f A.EdgeType
    , configFile  :: f FilePath
    , dotentity   :: f Bool
    , edgePattern :: f A.StyleName
    }

and then have:

defaultConfig :: Config Identity

Some other things that I don't like is the code duplication in src/Erd/Config.hs (e.g. in opts) and the Maybe in fmts, edges, edgePatterns.

I'll be happy to do this refactoring. However, I thought it should rather be done in a separate PR, because the changes are substantial and not directly related to #93. What do you think?

The configuration file shall be a separate matter.

I'm not sure if I understand. Do you want me to move the changes that make entries in the config file optional (88bcca2 and https://github.com/BurntSushi/erd/pull/95/commits/83585c172609965e733bb983f259669f178c336d) to a separate PR?

[mmzx]

@mmzx Thanks for checking the code and your comments.

separate PR would be nice to have.

Ok, I'll create it once this is merged.

Thanks!

It would be nice to improve on the solution that uses fromJust. - by not using fromJust itself.

Do you mean unsafeFromConfigOrDefault in app/Main.hs?

The solution using fromJust was there before my PR:

             , A.Splines $ fromMaybe (fromJust . edgeType $ defaultConfig) (edgeType conf)

Could you then just do one thing with it: change the name of the function unsafeFromConfigOrDefault to something that feels a little more trustworthy when the code is read?

I only extracted this into a function and gave it a name.

Sorry, my miscommunication! Processing the arguments is fine as it is.

This is one of the things that I had in mind when I said: "The whole configuration mechanism could, in my opinion, be improved, but - if it's needed - I'd rather do it in a separate PR." To repair this I'd have to change the type of Erd.Config.defaultConfig to get rid of Maybe from the default configuration. For example I can parametrize the type Erd.Config.Config:

data Config f = Config
    { cin         :: (String, Handle)
    , cout        :: (String, Handle)
    , outfmt      :: f G.GraphvizOutput
    , edgeType    :: f A.EdgeType
    , configFile  :: f FilePath
    , dotentity   :: f Bool
    , edgePattern :: f A.StyleName
    }

and then have:

defaultConfig :: Config Identity

Probably there is no need to make the type polymorphic, at least for now certainly not.

Some other things that I don't like is the code duplication in src/Erd/Config.hs (e.g. in opts) and the Maybe in fmts, edges, edgePatterns.

I'll be happy to do this refactoring. However, I thought it should rather be done in a separate PR, because the changes are substantial and not directly related to #93. What do you think?

Agree, it is unrelated to the current discussion.

[kukimik]

Could you then just do one thing with it: change the name of the function unsafeFromConfigOrDefault to something that feels a little more trustworthy when the code is read?

Changed it to fromConfigOrDefault.

My original intention was to stress that this function is not very trustworthy, as it may crash the program in runtime. AFAIK there is no naming convention for partial functions (see e.g. this discussion), so I used the unsafe prefix that stings the eyes - as a kind of "TODO" or "FIXME" note.

However, especially as this is not a library, perhaps you're right that it's better to get rid of the unsafe.

[kukimik]

Thanks. I don't have write access to this repo, so I can't merge the PR myself.