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

Added code for deletion of csv files in line #397 #12020

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

oge1ata
Copy link
Contributor

@oge1ata oge1ata commented Mar 26, 2024

Summary

References

Reviewer guidance


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... DEV: tools Internal tooling for development SIZE: very small labels Mar 26, 2024
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking on the right track - needs a little bit of cleanup, and I suggested one way it might be made easier!

@@ -392,6 +392,13 @@ def download_translations(branch, project, locale_data_folder):
code = lang_object[KEY_CROWDIN_CODE]
locale_dir_path = local_locale_path(lang_object, locale_data_folder)
logging.info("\tExtracting {} to {}".format(code, locale_dir_path))

#clear out existing files in langugage directory
for file_name in os.listdr(locale_dir_path):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably meant to be os.listdir?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is, apologies. Thanks for the correction.

#clear out existing files in langugage directory
for file_name in os.listdr(locale_dir_path):
file_path = os.path.join(locale_dir_path, file_name)
if os.path.isfile(file_path):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we are concerned only with CSV files here, we probably want to check that it is a CSV file before we delete it, just in case there are other files in this folder we don't want to delete!

Another way to do this would be to use the glob module instead of os.listdir, then you could just list out any existing CSV files and remove those: https://docs.python.org/3/library/glob.html#glob.glob

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh alright, I'd look into that thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just made the respective change with glob, could you check it out? Thank you

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good - but we need to cleanup the unnecessary files/changes before merge.

@@ -392,6 +393,11 @@ def download_translations(branch, project, locale_data_folder):
code = lang_object[KEY_CROWDIN_CODE]
locale_dir_path = local_locale_path(lang_object, locale_data_folder)
logging.info("\tExtracting {} to {}".format(code, locale_dir_path))

csv_files = glob.glob(os.path.join(locale_dir_path, '*.csv'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very neat!

] Outdated
@@ -0,0 +1,5 @@
Terminate batch job (Y/N)?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, I don't think we need any of the updates to any files apart from crowdin.py - could you remove this file, the package-lock.json, and the changes to package.json and yarn.lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay yes I would and then I'd push. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just did, could you confirm?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are still a couple of changes to package.json and yarn.lock.

You should be able to restore these to the default state on the develop branch if your develop branch is up to date with the upstream develop branch by doing this:

git checkout develop package.json yarn.lock

This will update the versions on your branch to the versions on develop - then commit the result so that there is no overall diff in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'd look into this. Thanks for the feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done now. My sincere apologies, I had network issues.

@github-actions github-actions bot added DEV: renderers HTML5 apps, videos, exercises, etc. DEV: frontend labels Apr 2, 2024
@rtibbles rtibbles marked this pull request as ready for review April 11, 2024 01:43
@rtibbles rtibbles removed DEV: renderers HTML5 apps, videos, exercises, etc. DEV: frontend SIZE: very large labels Apr 11, 2024
@rtibbles rtibbles merged commit 8c84488 into learningequality:develop Apr 11, 2024
31 checks passed
@rtibbles
Copy link
Member

Thank you for your contribution @oge1ata!

@oge1ata
Copy link
Contributor Author

oge1ata commented Apr 15, 2024

Thanks for your help and guidance as well @rtibbles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... DEV: tools Internal tooling for development SIZE: very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants