Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_service): configuration file shape #2825

Merged
merged 6 commits into from
Jul 6, 2022

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Jul 5, 2022

Summary

This PR closes #2823

I created a new data structure to track the configuration, called Configuration. This object is a separated entity from the WorkspaceSettings because the configuration that we will store in the json file is going to be different.

An example is the enabled field under formatter. Another example will be the extend field, which belongs exclusively to the configuration file.

For this reason, the resolution of the WorkspaceSettings will be done in a different step, where it will be joined with Configuration.

The first shape of the configuration file is the following:

{
  "root": true,
  "formatter": {
    "enabled": true,
    "formatWithErrors": true,
    "indentStyle": "tab",
    "indentSize": 2,
    "lineWidth": 80
  },
  "javascript": {
    "formatter": {
      "quoteStyle": "double"
    }
  }
}

Test Plan

I crated a small test to make sure that ALL fields are correctly mapped and read. Another one to make sure that the default values are correctly initialized by serde.

@ematipico ematipico temporarily deployed to aws July 5, 2022 14:54 Inactive
@ematipico ematipico requested a review from a team July 5, 2022 14:54
@ematipico ematipico temporarily deployed to aws July 5, 2022 14:55 Inactive
@github-actions
Copy link

github-actions bot commented Jul 5, 2022

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 5, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: e2daaaa
Status: ✅  Deploy successful!
Preview URL: https://199d0a75.tools-8rn.pages.dev
Branch Preview URL: https://feature-load-configuration-f.tools-8rn.pages.dev

View logs

@ematipico ematipico temporarily deployed to aws July 5, 2022 15:04 Inactive
@ematipico ematipico temporarily deployed to aws July 5, 2022 15:30 Inactive
indent_size: u8,

/// What's the max width of a line. Defaults to 80.
pub line_width: u16,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a bit confusing to have line_width be a raw u16 rather than just keeping it as a LineWidth.

If the reason for that is that LineWidth doesn't implement Deserialize, I think you can work around that with something like:

    /// What's the max width of a line. Defaults to 80.
    #[serde(with = "LineWidthDef")]
    pub line_width: LineWidth,
}

#[derive(Deserialize)]
#[serde(remote = "LineWidth")]
pub struct LineWidthDef(pub u16);

Copy link
Contributor

Choose a reason for hiding this comment

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

If you make the u16 in LineWidth private again (based on the comment from @leops), then I think something like this would still work:

    /// What's the max width of a line. Defaults to 80.
    #[serde(deserialize_with = "deserialize_line_width")]
    pub line_width: LineWidth,
}

fn deserialize_line_width<'de, D>(deserializer: D) -> Result<LineWidth, D::Error>
where
    D: serde::de::Deserializer<'de>,
{
    let value: u16 = Deserialize::deserialize(deserializer)?;
    LineWidth::try_from(value).map_err(serde::de::Error::custom)
}

You'd need to implement Display for LineWidthFromIntError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately doing so will force us to bypass the LineWidth::try_from. This feature requires having a impl From<LineWidthDef> for LineWidth, which goes in conflict with TryFrom

Copy link
Contributor

Choose a reason for hiding this comment

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

The second snippet I posted wouldn't require a new LineWidthDef struct at all. It's an alternative to the "remote" approach from that first snippet. It seems to work fine when I test it locally.

But I don't think it's a big deal if you want to keep this as line_width: u16
I'll just defer to @leops for this PR since he made this crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the easiest way to deal with this would be to implement Display for LineWidthFromIntError, then we may be able to simply write this:

Suggested change
pub line_width: u16,
#[serde(try_from = "u16")]
pub line_width: LineWidth,

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know about the serde try_from attribute. That's much better!

Copy link
Contributor Author

@ematipico ematipico Jul 6, 2022

Choose a reason for hiding this comment

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

Screenshot 2022-07-06 at 16 53 22

unknown serde field attribute `try_from`

Weird?

crates/rome_service/src/configuration/formatter.rs Outdated Show resolved Hide resolved
@ematipico ematipico temporarily deployed to aws July 5, 2022 16:45 Inactive
@ematipico ematipico requested review from leops and yassere July 5, 2022 16:45
@ematipico ematipico temporarily deployed to aws July 6, 2022 07:29 Inactive
@ematipico ematipico merged commit 6bb9100 into main Jul 6, 2022
@ematipico ematipico deleted the feature/load-configuration-file branch July 6, 2022 16:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create a de-serializable data structure to load via serde
3 participants