[FEATURE] Refactor for a clear, single-source of configuration for the bot.

[tomassirio]

[FEATURE] Refactor for a clear, single-source of configuration for the bot.

[rgroves]

@tomassirio a couple of questions:

1. Should I leave usage of process.env where configuration is actually realted directly to the environment and not exactly bot configuration — the one and only example is in mongoose.js where process.env.NODE_ENV is being used; that would be set in an external environment variable not in the bot config. I'm leaning toward leaving that as a valid usage of process.env and just adding some doc/notes in .env.example. What are your thoughts on this?
2. There are some usages of process.env in src/utils/encrypt.js but I'm not sure the encryption feature is fully baked yet. Should I go ahead and update those usages (process.env.KEY and process.env.IV) and maybe give them more distinct names (e.g. config.encryptionKey/config.encryptionIV)? Or should I let whoever is working on the encryption feature worry about updating this?

[tomassiriouala]

1. Definetely go the way you are leaning for. I'd rather have it documented on .env.example

2. The encryption feature is not yet fully baked. I haven't heard from the developer on the PR for a while. I'd go with the way of distinctive names for the .env variables. As we spoke before i'd rather have declarative environment variables

[rgroves]

I hesitate to change the encrypt.js file if it's not fully baked, but if you want me to I will.

Another question, is the code in src/commands/info/commands.js actually used by the bot? Unless I've missed something, I don't think it is. Should it be removed?

I've made changes to files using process.env and am trying to do testing before committing and submitting a PR to ensure I did not introduce any errors and I found the above (and encrypt.js) which I can't really test because they are not really in use.

[tomassirio]

I think that the commands in info was implemented some time during development but was discarded towards the help command. If there's no use to it, then delete it

[tomassirio]

I merged just now the encryption PR. Check it out

[rgroves]

I like to keep changes in PR related to issues they resolve, so I'll create an issue for removal of src/commands/info/commands.js and we can leave that one for an easy win for a first-timer or Hacktoberfest participant.

I'll take a look at the enrcryption PR in a bit.

[rgroves]

@tomassirio There seem to be a bunch of linting issues with the code that was merged for the encryption PE. So now anyone branching off or merging in the current point in master will have those same linting issues when they try to issue a PR. Those issues should probably be cleaned up before further work is done.

Are there any guidelines in place to ensure that the master branch always stays clean? If not, should there be? Maybe the contributor guidelines should be filled out a bit to make sure people know how to merge the current master into their working branch and resolving any issues before issuing a PR? Though they'd have to be enforce to be of real use, not sure if you're interested in doing that as the repo owner. Let me know your thoughts, if you'd like I can help flesh out better contributor docs to help explain the workflow that should be followed to avoid the messiness when it comes to getting a PR accepted.

[tomassirio]

There should be a guideline as to how you should push to master. I think the elint job didn't trigger with that last PR.

How do you think we could make the contributor docs better?

[rgroves]

I was thinking add content to make things clearer and more guided for newcomers/beginners and to make things easier for you in managing PRs.

For instance, right now the contributor page just has the Contributor Covenant on it, but no actual info about making contributions specifically in this repo.

The main README has a Contribution Guidelines with some basics, but I don't think those are even being followed currently based on looking at some of the PRs. For instance the guildelines say to name your branch change/username and to use the sign-off switch when committing, but looking at some accept PRs those don't look to be followed much.

The contributor doc could be made better by adding info about:

• how to add the original repo as a remote (for syncing)
• how to sync their local repo's master and working branch with changes from the upstream master
• how to perform a final sync and lint check before issuing a PR
• how to handle a PR that has been submitted, but ends up having conflicts due to other PRs being merged before theirs

That kind of stuff that will help make PRs cleaner so that when you merge them it doesn't throw the master branch into a state of disarray.

Getting a test suite in place would help also, and I believe someone is working on that. But those are the ideas I was thinking of for better contributor doc.

BTW, I'll have a PR for this issue sometime today.

[tomassirio]

Amazing. I'm learning a lot from this issues you are talking about. I'll try to cover my side from the PR's acceptance