-
Notifications
You must be signed in to change notification settings - Fork 7
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
added constants.py and test_constants.py #28
Conversation
Connects to issue #15. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @wolff01, there are a significant number of changes that you need to make to this PR. Primarily, there are a number of constants defined inside of this module that are not absolutely needed. Please delete all of the constants that are not needed. If a constant is currently inside of this module then it needs to be clearly used in the existing source code of the Cellveyor tool.
@wolff01, would you also be able to elaborate on the PR and provide a description--make it super clear to all reviewers (which will include at least one other student) what the intent of the PR is? |
Made more changes to the constants.py file that are needed for cellveyor. There are possibly more to adjust I wanted to make sure that we have the constants.py for cellveyor, I know @hayleepierce is working on the logging, we have collaborated and come up with the solution that I push this while she works on logging and creates that in the constants.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @wolff01, please note that we can't merge this PR until it is clear that each of the constants that you are proposing is actually used in the application's source code. If you have questions about this, please check in with me or @jnormile or @hayleepierce and we can clarify further what we are requesting for you to change!
The constants file is complete for a base usage. For anymore data that needs to be added for anyone else you should be set to go. |
The constants.py added notation for indentation and insertion of a new line when called in any area in the markers section. Also stored within is a Human readable boolean and an overview of Cellveyor with basic information and as well the filesystem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing convincing evidence that the constants defined in constants.py
are being used outside of constants.py
. As the PR currently stands, it looks like it's adding a lot of code that isn't necessarily doing anything.
Could you elaborate on why you believe using constants.py
to be a good idea? The current implementation doesn't convince me.
@jnormile The constants.py is needed in order to store values and not have the worry of accidentally changing the values stored within it in the main code. It will help improve readability and be better to maintain than not having it. The values inside of the constants.py file can be properly stored instead of having an extensive amount of data in our main. |
More data will be added for certain classes as well as new classes will be added. This base is to give use a solidified class Cellveyor as well as other small things such as a human readable boolean and markers like indentation and new line notation. |
To be clear, my main concern with this PR is that it adds a lot of code that isn't actually in use. From what I can tell, there's only one line in If we're going to use the constants approach you're proposing, then I ask that you take up the suggestion made by @hayleepierce and actually integrate the constants you're creating throughout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all your work on this issue!
@wolff01 Please don't forget to sync your fork! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cutting this down in size to help ensure its more maintainable! Approved!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
No description provided.