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

daveroga/rationalize configs #398

Merged
merged 2 commits into from
Feb 14, 2022
Merged

daveroga/rationalize configs #398

merged 2 commits into from
Feb 14, 2022

Conversation

daveroga
Copy link
Contributor

@daveroga daveroga commented Jan 11, 2022

Fixes issue #355 .

The configuration for this application is becoming fragmented. It now exists in:

  • Some constants.mjs files.
  • Environment files
  • config module files
  • env-cmd module files

Probably it is sensible just to use one of these.

Steps to solve this:

  • Used config module files for tests and removed constants.mjs.
  • Removed env-cmd module files.
  • Tried to use config module in cli but then found some problems with babel/webpack and the wallet. Some constants are cli specific and should be in the cli project.

@daveroga daveroga self-assigned this Jan 11, 2022
@daveroga daveroga linked an issue Jan 11, 2022 that may be closed by this pull request
config/default.js Outdated Show resolved Hide resolved
Copy link
Contributor

@druiz0992 druiz0992 left a comment

Choose a reason for hiding this comment

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

I would like to keep web3 node url out of repo code. PR #334 proposes a way to do it. I suggest once it is approved, just reuse it

@daveroga
Copy link
Contributor Author

PR already solved but e2e test failing due to issue #427 that should be solved first.

@daveroga
Copy link
Contributor Author

daveroga commented Feb 8, 2022

ganache-test seems to fail sometimes. Forced modify a file to test it again.

@ChaitanyaKonda
Copy link
Contributor

This seems to add 263 commits in total. Most of which are commits that already exist in the master.
Merging this has the risk of breaking the code and adding back logic that we removed.

Could you revert this to just the commits from this PR please?

@ChaitanyaKonda ChaitanyaKonda added the DNM Do not merge label Feb 9, 2022
@daveroga daveroga force-pushed the daveroga/rationalise-configs branch from c7dba3c to b55fd39 Compare February 9, 2022 11:31
@daveroga daveroga force-pushed the daveroga/rationalise-configs branch from b55fd39 to 5e9e4c7 Compare February 9, 2022 11:34
@daveroga
Copy link
Contributor Author

daveroga commented Feb 9, 2022

Wrong commits added after rebase were reverted and rebase again from master.

@daveroga daveroga removed the DNM Do not merge label Feb 10, 2022
@daveroga daveroga mentioned this pull request Feb 14, 2022
Copy link
Contributor

@signorecello signorecello left a comment

Choose a reason for hiding this comment

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

LGTM but just mind some tests are being removed by #490

@daveroga daveroga merged commit 8a213cd into master Feb 14, 2022
@github-actions
Copy link

🎉 This PR is included in version 1.16.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

signorecello added a commit that referenced this pull request Feb 14, 2022
…basis ⚔

fix: fixing conflicts with #398 and moving eslint rule to a per-file basis ⚔
@daveroga daveroga deleted the daveroga/rationalise-configs branch March 1, 2022 18:54
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.

Rationalise configs
4 participants