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

MAINT: Cleaning up some repo files #316

Merged
merged 3 commits into from
Jan 2, 2023

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

Please check the type of change your PR introduces:

  • GitHub maintenance

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • ReadMe, Docs and GitHub maintenance:

    • Spelling has been verified
    • Code docs are working correctly

What is the current behavior?

When trying to download (i.e. to clone) the RocketPy repository, usually the user also downloads several files that he/she will probably not use. Some of the files were added in the first upload of this repo and have never been used since then.

What is the new behavior?

38 data files are being deleted from this repo. I'm saving them in our organizational gdrive account, so we never lost it.
Cloning the repo should be easier now.

Does this introduce a breaking change?

  • Yes
  • No

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Gui-FernandesBR
Copy link
Member Author

This is part 1 of issue #281

@Gui-FernandesBR
Copy link
Member Author

Kinda bothers me how the thrust file for the keron motor is repeated 3 times in the repo (1 for tests,1 for data/motors, and 1 for dispersion analysis), even if it is a small file, it is annoying.
Accepting suggestions if they are easy to implement, otherwise let's just move forward.

@MateusStano
Copy link
Member

Kinda bothers me how the thrust file for the keron motor is repeated 3 times in the repo (1 for tests,1 for data/motors, and 1 for dispersion analysis), even if it is a small file, it is annoying. Accepting suggestions if they are easy to implement, otherwise let's just move forward.

I think it would be better if every data files were only found inside the data folder and everything has to access to them from there.

For the tests to work are the data files required to be inside the tests folder? If so then I would suggest that we leave the keron file inside the tests folder, which is better than repeating the file more than once.

If the tests don't require the files to be inside the tests folder then I think is better to create a standard and leave all data-related files inside the data folder and remove them from the fixtures folder

@Gui-FernandesBR
Copy link
Member Author

For the tests to work are the data files required to be inside the tests folder?

Not, it is not. Indeed, some of the tests already uses the files available at data folder.
I agree on moving data from tests to data folder. Let's see what @giovaniceotto think about it, since he was the one who created the fixtures.

Also, I think that the data folder should contain only raw data files, I mean, mainly .csv or .json file.
Any code (.py or .ipnb) should be at examples or docs.

@Gui-FernandesBR Gui-FernandesBR merged commit 37d147c into master Jan 2, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the maint/cleaning_repo_files branch January 2, 2023 01:55
@giovaniceotto
Copy link
Member

giovaniceotto commented Jan 2, 2023

Sorry guys... Was this branch merged into master without any tests passing?

Screenshot_20230101_235808_GitHub.jpg

@Gui-FernandesBR Gui-FernandesBR restored the maint/cleaning_repo_files branch January 2, 2023 03:58
@Gui-FernandesBR
Copy link
Member Author

Sorry guys... Was this branch merged into master without any tests passing?

Screenshot_20230101_235808_GitHub.jpg

If I understood correctly, the problem with tests were related to the timezonefinder issue.
Codes were running locally.
None of the deleted files were being used by tests of notebooks.
None of the tests or codes were modified.

Nevertheless, I restored the branch if you want to further investigate.

@Gui-FernandesBR Gui-FernandesBR deleted the maint/cleaning_repo_files branch January 7, 2023 02:29
@Gui-FernandesBR Gui-FernandesBR restored the maint/cleaning_repo_files branch January 7, 2023 02:29
@Gui-FernandesBR Gui-FernandesBR deleted the maint/cleaning_repo_files branch January 22, 2023 18:27
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.

3 participants