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

Fail upon encountering unrecognized fields when parsing config structures #697

Merged
merged 3 commits into from
Sep 8, 2024

Conversation

fsoikin
Copy link
Contributor

@fsoikin fsoikin commented Sep 7, 2024

lib/src/PackageSet.purs Outdated Show resolved Hide resolved
spago.yaml Outdated Show resolved Hide resolved
spago.lock Outdated
- variant
test_dependencies:
- spec
core:
Copy link
Member

Choose a reason for hiding this comment

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

We should also regenerate the lockfile using spago 0.93.19

@thomashoneyman thomashoneyman merged commit 7a42ca1 into purescript:master Sep 8, 2024
16 checks passed
@fsoikin fsoikin deleted the fail-on-bogus-fields branch September 9, 2024 02:46
@f-f
Copy link
Member

f-f commented Sep 9, 2024

@thomashoneyman could we revert this one? We are introducing strict parsing of the Manifest, Metadata, which breaks forward compatibility guarantees (i.e. older versions of this library being able to parse files produced with newer versions which might introduce new fields.

@fsoikin if the intention is to support purescript/spago#1165 then we don't need any of these changes: the Spago config only needs strict record parsing for the Location type. The other types we are using in the Config parsing are newtypes so are not affected by strict record parsing.

@thomashoneyman
Copy link
Member

Ah, I was just thinking about backwards compatibility. We can revert.

@fsoikin
Copy link
Contributor Author

fsoikin commented Sep 9, 2024

The Location type does need strict parsing because the subdir field is optional and therefore subject to typos. But what about backwards compatibility? The same codec is used for network communication too, right? Maybe have too codecs - Location.codec and Location.codecStrict?

@f-f
Copy link
Member

f-f commented Sep 9, 2024

I think we should let the registry do its thing and just have the strict codecs we need in spago itself - the codecs don't need to be colocated with the types

@fsoikin
Copy link
Contributor Author

fsoikin commented Sep 9, 2024

But then subdir is subject to typos, which is what purescript/spago#1165 is complaining about.

@fsoikin
Copy link
Contributor Author

fsoikin commented Sep 9, 2024

Could be n misspelled subDir or sub_dir, or even ref by mixing up different formats of similar purpose.

@f-f
Copy link
Member

f-f commented Sep 9, 2024

We can use strict codecs in spago and avoid the typos, but we should just duplicate the codec there instead of changing this one. We don't have the same forwards compatibility concerns in spago itself (we do have some, but just for a subset of the config file)

@fsoikin
Copy link
Contributor Author

fsoikin commented Sep 9, 2024

Ah, I see the constructors are exported. Somehow I thought they weren't, similar to Version and PackageName.

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.

Issue warnings when the configuration has unrecognised fields
3 participants