Skip to content
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.

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

Allow for alternate restyled config locations #101

Closed
wants to merge 3 commits into from
Closed

Allow for alternate restyled config locations #101

wants to merge 3 commits into from

Conversation

chiroptical
Copy link
Contributor

This PR allows for alternate restyled.yaml configuration files closing #98

I formatted the code with brittany using your configuration file.

I tried to write the most general findJust function possible. I tried two versions shown here: https://gist.github.com/chiroptical/2a921faa08976f6a176557fe57bb6021.

I would like to add a test to make sure it is working correctly.

@CLAassistant
Copy link

CLAassistant commented May 8, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

src/Restyler/Config.hs Outdated Show resolved Hide resolved
@pbrisbin
Copy link
Member

pbrisbin commented May 8, 2020

For testing, I think you should start here https://github.com/restyled-io/restyler/blob/master/test/Restyler/ConfigSpec.hs#L20.

Apologies for not test-compiling anything I'm about to write, but I'd imagine you could do something like:

spec = do
  it "whatever" $ do
    app <- liftIO $ testApp "/" [("/a", "Some YAML"), ("/b", "Other YAML")]
    x <- runRIO app
        $ tryTo showConfigError
        $ loadConfigFrom ["/c", "/b"]
        $ const
        $ pure testRestylers

    -- assert you loaded /b, since /c didn't exit

    y <- runRIO app $ do
        writeFileUnreadable "/x" -- if /x is read, you will explode
        tryTo showConfigError
          $ loadConfigFrom ["/a", "/x"]
          $ const
          $ pure testRestylers

   -- assert you loaded /a and did not look at /x

Copy link
Member

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM!

Did you want me to wait for tests before merging, or add them myself after?

@chiroptical
Copy link
Contributor Author

It took me a bit to figure out how I actually make this work. Let me know what you think of these. One other trivial test (if you think it is necessary) would be to add two configs to the test filesystem and make sure it reads the "first" one.

Copy link
Member

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great! I'm going to pull it into another branch and do some minor tweaks before merging.

@chiroptical
Copy link
Contributor Author

chiroptical commented May 13, 2020

It looks like it wasn't too much extra work to clean this up, thanks! See #103. I guess this can be closed.

@chiroptical chiroptical deleted the alternate_restyle_configs_locations branch May 13, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants