Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

Ignore null/undefined config objects and return a useful error about the file with the problem #2633

Merged
merged 4 commits into from
Dec 3, 2021

Conversation

evantahler
Copy link
Member

@evantahler evantahler commented Dec 2, 2021

  1. If there are null or undefined config objects, we now return an error with the filename that has the problem (rather than mysteriously crashing)
  2. If there's a problem saving the Sample Record, don't write null to the config file

This PR also changes config object writing (specifically for records, but it may apply in the future to other config types). This allows for merging config objects that share a destination filepath.

What this allows us to do is break out Record config files on a per-model basis

Before:

config/development/records.json

After:

config/development/customers/records.json
config/development/accounts/records.json

Thanks to @rwfeather!

Checklists

Development

  • Application changes have been tested appropriately

Impact

  • Code follows company security practices and guidelines
  • Security impact of change has been considered
  • Performance impact of change has been considered
  • Possible migration needs considered (model migrations, config migrations, etc.)

Please explain any security, performance, migration, or other impacts if relevant:

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached where applicable.
  • Relevant tags have been added to the PR (bug, enhancement, internal, etc.)

@evantahler evantahler added the bug Something isn't working label Dec 2, 2021
@evantahler evantahler marked this pull request as ready for review December 2, 2021 22:42
Copy link
Member

@pedroslopez pedroslopez left a comment

Choose a reason for hiding this comment

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

Few questions mainly to understand what was causing the null objects and to make sure it was being addressed since, while I see errors being handled on the config loader, the generation of record config objects seems like it hasn't changed.

If my assumptions are correct, looks good!

core/src/modules/configWriter.ts Show resolved Hide resolved
core/src/modules/configWriter.ts Show resolved Hide resolved
core/src/modules/configWriter.ts Show resolved Hide resolved
@evantahler evantahler merged commit 9dda14e into main Dec 3, 2021
@evantahler evantahler deleted the 180510967-fix-null-config-objects branch December 3, 2021 16:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants