Skip to content
This repository has been archived by the owner on Nov 16, 2020. It is now read-only.

Solution to issue #11 #16

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Solution to issue #11 #16

wants to merge 18 commits into from

Conversation

Shankhanil
Copy link
Contributor

@Shankhanil Shankhanil commented Apr 23, 2020

Solution to issue #11
@AlwaysLivid Check out the code. Also, there is some merge issues. Any help?

@Shankhanil Shankhanil marked this pull request as ready for review April 23, 2020 17:07
@Shankhanil
Copy link
Contributor Author

I have resolved the merge conflicts. Review the code @AlwaysLivid

@n0toose
Copy link
Owner

n0toose commented Apr 24, 2020

If you could squash the commits before pushing them, that'd be strongly appreciated. I'll look into it in a bit.

@n0toose
Copy link
Owner

n0toose commented Apr 24, 2020

Your pull request seems a bit broken-- the merge resolution doesn't quite seem to have been completed.

The variables and all revelant checks should be loaded before creating a new instance of the Twitter bot, so that the only authentication-related exceptions that could occur during the execution of the bot would be either incorrect credentials or ratelimits. Putting everything under an exception handler doesn't seem quite clean to me. Also, make sure that the format of your messages match the clear clear format of the other messages.

Also, please make the change a bit more inclusive and accepting of other continuous integration environments as well. Maybe adding a new function that checks for a variable that's used across platforms (e.g. Travis-CI, CircleCI, etc.) would be a good idea. I can't recall whether CONTINUOUS_INTEGRATION and/or CI work.

@n0toose
Copy link
Owner

n0toose commented Apr 24, 2020

You also seem to be refactoring the checks for whether a person is using a CI or not, which is irrelevant to the issue that needs to be addressed.

The only thing that's necessary for the resolution of #11 is adding another case exception, in case everything else fails, unless if you're in a CI environment. So, it'd be best to build that upon existing code rather than rewriting everything.

Please note that I may have made inaccurate remarks, because the merge conflict resolution seems to be broken, which is confusing me.

@Shankhanil
Copy link
Contributor Author

Your pull request seems a bit broken-- the merge resolution doesn't quite seem to have been completed.

The variables and all revelant checks should be loaded before creating a new instance of the Twitter bot, so that the only authentication-related exceptions that could occur during the execution of the bot would be either incorrect credentials or ratelimits. Putting everything under an exception handler doesn't seem quite clean to me. Also, make sure that the format of your messages match the clear clear format of the other messages.

Also, please make the change a bit more inclusive and accepting of other continuous integration environments as well. Maybe adding a new function that checks for a variable that's used across platforms (e.g. Travis-CI, CircleCI, etc.) would be a good idea. I can't recall whether CONTINUOUS_INTEGRATION and/or CI work.

AppVeyor: CI, APPVEYOR
Azure Pipelines: TF_BUILD
CircleCI: CI, CIRCLECI
GitLab CI/CD: CI, CI_SERVER, GITLAB_CI
Travis CI: CI, CONTINUOUS_INTEGRATION, TRAVIS
Here are the environment variables for most popular CI . Should I check for all of them? @AlwaysLivid

@n0toose
Copy link
Owner

n0toose commented Apr 28, 2020

Here are the environment variables for most popular CI . Should I check for all of them? @AlwaysLivid

Sure, as long as they are in a single array variable and indented properly. I may add support for other CIs later on as another "task".

@Shankhanil
Copy link
Contributor Author

The only thing that's necessary for the resolution of #11 is adding another case exception, in case everything else fails, unless if you're in a CI environment. So, it'd be best to build that upon existing code rather than rewriting everything.

I am confused regarding this. Why do we need another exception case? There already is an exception case which handles authentication error. All we need to do is check if the bot is in CI or not. If in CI, throw some message and terminate, else get the credentials list from the config file. Isn't that the correct approach?

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

Successfully merging this pull request may close these issues.

None yet

2 participants