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 in the config #1272

Merged
merged 12 commits into from
Sep 12, 2024

Conversation

fsoikin
Copy link
Collaborator

@fsoikin fsoikin commented Aug 23, 2024

Description of the change

(partially) fixes #1165.

The codec library doesn't have such feature, so I basically had to reimplement the relevant functions.

Unfortunately, this change doesn't affect some sections of the config, because their types and codecs are defined in the Registry library. I am planning to contribute the change back to the codec library and then migrate the Registry library to it.

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • [ ] Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

@@ -38,7 +38,6 @@ package:
bundle:
type: "app"
minify: true
sourceMaps: true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And immediately we have a winner!

Turns out spago bundle has a command-line parameter for source maps, but not a config parameter. Another bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or not a bug? Technically this can be specified in extra args, as I did here, but this is asymmetry. Not sure what the overarching philosophy is here.

Copy link
Member

Choose a reason for hiding this comment

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

Great catch!

Overarching philosophy is that we try to keep flags and config params as close to each other as possible. There are some notable exceptions, but this one is a bug: we should have a config parameter as well.

@f-f
Copy link
Member

f-f commented Aug 23, 2024

P.S. your commit messages for the autoformatter are extremely amusing, please keep them coming ❤️

@f-f
Copy link
Member

f-f commented Aug 23, 2024

Code looks good - we shall wait for the upstream codec-json lib to merge the patch so we don't have to keep the library code in here

@fsoikin
Copy link
Collaborator Author

fsoikin commented Sep 9, 2024

Alright, upstream libraries finally updated, this should be ready now.

Sorry for the force-push, somehow my history wasn't matching GitHub, I must have rebased long ago, when I touched this PR last time.

I decided not to port:

  • Codecs under Docs.Search, because the JSON files there are not written by the user.
  • Spago.Config.remotePackageSetCodec - this is used to read package set from both local files and network responses, and I think that making it strict would break backward compatibility: if the server adds new fields, old clients won't be able to read responses anymore. Plus, there is no danger of typos here, because all fields are mandatory.
  • Spago.Purs.moduleGraphNodeCodec and codecs in Spago.Purs.Graph - same reason. This is used for reading purs responses.
  • Spago.Command.Ls.formatPackagesJson.packageCodec - used only for output.
  • Spago.Command.Registry.search.infoDataCodec - used only for output.
  • Codecs in Spago.Psa.Types - used to parse PSA output, same reason as for purs and remote registry.

spago.lock Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what's going on here. Looks like it used to be JSON, but now it's YAML. I don't think I changed anything in that regard.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I see, this is #1280

spago.lock Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, this is looking better now.

spago.yaml Outdated
registry-lib:
git: https://github.com/purescript/registry-dev.git
ref: be0d7ffd1c16aa70e8f065a928e941ebc053c013
ref: 7a42ca1381ab9e1203a9f89d077d47d41ec59316
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 use the latest, fc203a9e2a0b96a90ace20dc6958c157cbbca16b

@f-f f-f merged commit 99c485d into purescript:master Sep 12, 2024
5 checks passed
@fsoikin fsoikin deleted the fail-on-bogus-fields branch September 12, 2024 12:45
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