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

Add files via upload #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add files via upload #2

wants to merge 1 commit into from

Conversation

brodkemd
Copy link

Here are the changes I have made. Let me know if anything needs work or if you want anything organized differently.

Things I added:

  • imports methods from local files by creating a list of all of the functions and their corresponding files

Things to ignore:

  • ignore the functions "remove_return" and "replace_keywords_in_line" these are temp fixes and are not important

Notes:

  • I have also been very busy lately so the code isn't great but I will polish it and make it better
  • I am also in the process of creating a function that will add some functionality to the array methods that are added

Thanks!

Marek

@brodkemd
Copy link
Author

I am not expecting you to merge this, I'm just looking for feedback.

@ebranlard
Copy link
Owner

Hi Marek,
I had a quick glanced (sorry for the delay), and look good. I have the following suggestions:

  • place your file marek_func into the folder matlab_parser and give it a name reflecting the content of this file (maybe import_handler.py).
  • add some tests in the directory test. You'll find examples in this directory. The file tests\test_files.py converts some of the files located in tests\files\. Other files in the tests folder are more "unit tests", testing smaller functions. You can add some unit tests for your import_handler for instance.

If you can take care of that, I'll be happy to merge your pull request.

Sorry again for the long delay.

@ebranlard ebranlard changed the base branch from master to main November 1, 2022 18:18
@ebranlard
Copy link
Owner

Hi @brodkemd
Do you think you'll have time to address some of the recommendations I suggested?
Thanks lot!

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.

2 participants