Skip to content

Addition of the first block of code to implement the registering of new experiments #9

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.

Sign up for GitHub

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

Jump to bottom
Open
aOlmo wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Addition of the first block of code to implement the registering of new experiments #9

aOlmo wants to merge 2 commits into from

Conversation

@aOlmo
Copy link

@aOlmo aOlmo commented Aug 8, 2017

This Pull Request is made to add the first block of code that the previous contributor created while working in the functionality of registering of a new experiment.

As a heads up, there are some parts in the PR code that will be changed or erased in the next commits. To see which ones are and all the changes included in this PR, please, look at the last commit message. Finally, this PR may not work "as is" now but I consider it to be the fundamental base code for the future ones.

PS: I had to create a new PR because I had to change the commit messages and also some code, so please disregard the last PR I made.

@aaaaalbert
Copy link

aaaaalbert commented Aug 10, 2017

A few suggestions:

  • Commit messages must be more descriptive than "Adding old code". Why is old code added? What does it do? (It adds create_... functions for users and sensors, right?)
  • Following the above, if your commit message for 3f179c2 has all this information, then feel free to squash before sending the PR. See the Git docs for "rewriting history".
  • Our coding style guidelines suggest two spaces per indent level. I see four in some places.
  • Docstrings are incomplete, e.g. here and here.
  • Consider making for field in [precision, goal, ...]: ... loops out of repeated blocks like these.
  • 64 bytes for a street address is not too generous.
  • Take care of maximum line lengths, e.g. here.
  • Typo: "precision".
  • I don't understand the comments for the ..._other properties of Sensor, such as this. We store what we don't support?
  • Are these default values ways of allowing or disallowing a sensor? Does the researcher request access to a sensor by providing True? The comments don't quite tell me.
  • Typo: "concrete".
  • What about the many changes of file access bits? What's the purpose?

@aOlmo
Copy link
Author

aOlmo commented Aug 10, 2017

Thanks a lot for the feedback, I will be working on all of this and will send another PR as soon as I have it.
I just have a couple comments about what you mentioned:

In regards to this comment:

  • Typo: "concrete".

This is solved later in another commit where I had to change several other things due to that renaming.

Sorry, I did not quite understand this last comment:

  • What about the many changes of file access bits? What's the purpose?

Thanks again for the feedback,

@aaaaalbert
Copy link

aaaaalbert commented Aug 10, 2017

concret

Thanks, no problem.

File access bits

See the "Files" view of the PR, https://github.com/SensibilityTestbed/clearinghouse/pull/9/files

Scroll down a bit until you see common/util/nodestatus.py 100755 → 100644. The number--arrow--number notation informs us that the access rights granted on that file have been changed. (See man chmod for reference). I wonder why that happened.

Eric Velazquez and others added 2 commits August 14, 2017 10:46
@aOlmo
Add the first block of code to implement the registration of an exper…
c382868 
…iment

- Add the new function create_experiment(): The main purpose of this function is
  to add a new experiment in the database.
- Add new functions to interface.py:
   - register_experiment: This function will add a new experiment to the database
     by using the methodcreate_experiment() in maindb.py
   - register_sensor: Function that adds a new sensor to the database calling the
     create_sensor method in maindb.py
- Add the following classes to models.py:
   - Experiment: Class that will save the basic data of a new experiment created
     by a researcher
   - Sensor: Abstract class where all sensors will inherit from. This saves the
     general data from each sensor: experiment_id, frequency, frequency_unit …
   - Battery, Bluetooth, Cellular, Location, Settings, ConcretSensor,  Signal_strengths,
     Wifi: All these classes are the ones that inherit from the Sensor class
     and will store their specific values.
- Add new forms RegisterExperimentForm, GeneralSensorAtributesForm and all sensor
   forms (BatteryForm, BluetoothForm, …) to the file website/html/forms.py
- Add more django blocks to website/html/templates/control/control_base.html
- Add file website/html/templates/control/registerexperiment.html This file is
  currently very improvable. In later commits this one is completely changed
  but for the moment it is necessary to have everything working together.
- Minor changes in indentation and spacing in website/html/urls.py
- Add registerexperiment function to website/html/views.py: This function
  registers a new experiment including all the sensors that a reasearcher
  might have chosen. This function although seems to work is very badly written
  and will be changed in the following commits.
- Changes in website/settings.py: Changes in the INSTALLER_BUILDER and addition
  of the new model: clearinghouse.website.control.models
@aOlmo
Change indentation and access rights from 644 to 755
84962a0 
- Change indentation in all files inside "website" folder to be two spaces
  per each indentation level.
- Change in user bit rights from 644 to 755 to several files which were
  previously changed.
@aOlmo
Copy link
Author

aOlmo commented Aug 14, 2017 •
edited
Loading

Here are the new changes in this PR:

  • Commit messages must be more descriptive than "Adding old code". Why is old code added? What does it do? (It adds create_... functions for users and sensors, right?)
  • Following the above, if your commit message for 3f179c2 has all this information, then feel free to squash before sending the PR. See the Git docs for "rewriting history".

Those three last commits have been squashed into c382868 now. The commit message is the same as the last commit of those three.

  • Our coding style guidelines suggest two spaces per indent level. I see four in some places.
  • What about the many changes of file access bits? What's the purpose?

Indentation has been changed to be 2 spaces per each indentation level and the file access bits are now the same as previously (I don't know why these were changed). This can be seen here

Changes that have been made following the suggestions and will come in the next PRs:

  • Docstrings are incomplete, e.g. here and here.

Those are now more complete here and here

  • Consider making for field in [precision, goal, ...]: ... loops out of repeated blocks like these.

Done here

  • 64 bytes for a street address is not too generous.

In this commit, all byte lengths have been reviewed and changed to the current standards following the second recommendation here

  • Take care of maximum line lengths, e.g. here.

Done in this commit

  • Typo: "precision".

Corrected here

  • I don't understand the comments for the ..._other properties of Sensor, such as this. We store what we don't support?

The comments on "_other" properties of Sensor have been rewritten

  • Are these default values ways of allowing or disallowing a sensor? Does the researcher request access to a sensor by providing True? The comments don't quite tell me.

I have tried to explain it a little bit better in the docstrings from each class like the Battery one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aOlmo @aaaaalbert