Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Move exceptions.js file to be part of config.json to simplify config structure #3309

Closed
nazarhussain opened this issue Apr 12, 2019 · 5 comments · Fixed by #3451
Closed

Move exceptions.js file to be part of config.json to simplify config structure #3309

nazarhussain opened this issue Apr 12, 2019 · 5 comments · Fixed by #3451
Assignees
Labels

Comments

@nazarhussain
Copy link
Contributor

Expected behavior

There should be only one configuration file for one network.

Actual behavior

Right now we have one file exceptions.js for each network, which actually is part of configurations at key modules.chain.exceptions. Using the following code;

https://github.com/LiskHQ/lisk/blob/c75591a52864d408828c29d410859902c5bffd13/src/index.js#L79

To avoid any confusion we should merge the exception file into actual config.json for each network and remove the above code line. This will simplify the config structure and help to understand it easily that exceptions are actually options for chain module.

Which version(s) does this affect? (Environment, OS, etc...)

1.7

@pablitovicente
Copy link
Contributor

@SargeKhan is working in exceptions now @nazarhussain so you might want to check with him before going ahead with this one

@nazarhussain
Copy link
Contributor Author

The change here has no relation to how we process the exceptions. Its just the matter of keeping a separate file.

@nazarhussain nazarhussain self-assigned this Apr 16, 2019
@nazarhussain
Copy link
Contributor Author

Although the change was straight forward but just noticed one edge case. The exceptions file is a js file and config file is json file. In exceptions file we have some keys as integers while in JSON we can't have keys as integers.

So if we move the exceptions to config file, we have to update the usage of exceptions accordingly.

@nazarhussain
Copy link
Contributor Author

@shuse2 There are a lot of comments in the exception files

https://github.com/LiskHQ/lisk-sdk/blob/265f1cf934d87000248ef572157c668041bf6543/lisk/config/mainnet/exceptions.js#L22-L25

And these are useful for our reference. If I move exceptions to config.json we have to remove these comments, as JSON files don't allow comments. On the other hand I don't see it right to convert config.json to a script file. It behaves same for the NodeJS environment, but not compatible for other utilities and linux command lines e.g. cat config.json | jq .. Its a data file and should remain as data file.

So what direction we should now?

  1. Move the exceptions to config.json file and remove the comments?
  2. Keep the exceptions file as it is?

@MaciejBaj The issue was intended by your directions to have only one config file for each network. So your opinion is also required on it.

@shuse2
Copy link
Collaborator

shuse2 commented Apr 25, 2019

After discussion, we will convert it to JSON, but document the details in README

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants