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

Fix github url strings (org edx -> openedx) #63

Merged
merged 3 commits into from
Sep 14, 2022
Merged

Conversation

sarina
Copy link
Contributor

@sarina sarina commented Sep 7, 2022

This PR was autogenerated

This pr replaces the old GitHub organization, github.com/edx, with the new GitHub organization, github.com/openedx.

Tagging @openedx/tcril-engineering for review, but others are welcome to provide review.

Ref: openedx/axim-engineering#42

@sarina
Copy link
Contributor Author

sarina commented Sep 13, 2022

@Jawayria or @rayzhou-bit - since you've last committed to this repo could you help me here? I updated pylintrc and get the following error:

************* Module pylint_django
pylint_django:1:0: F5101: Django is not available on the PYTHONPATH (django-not-available)

From a quick Google it seems DJANGO_SETTINGS_MODULE may need to be defined in tox.ini but I'm not familiar with this project about how that should be defined. In the cookiecutter repo, it points to test_settings.py, which is also not defined in this repo.

Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Looks close. Minor fix needed.

pylintrc_backup Outdated
@@ -0,0 +1,387 @@
# ***************************
# ** DO NOT EDIT THIS FILE **
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this file, and also add it to .gitignore?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, not sure if you can do this for all your PRs that have this problem.
FYI: @nedbat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this comes when you do an edx_lint write <file> but the last time it was edited, it was done manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ned brought up that this is a temporary file that isn't meant to be committed, and we discussed possible solutions around this using .gitignore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely don't commit this file. Ideally use it to figure out what previous hand-edited change needs to be moved to a pylintrc_tweaks file.

I'm conflicted about whether to add it to .gitignore

Copy link
Contributor

Choose a reason for hiding this comment

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

@sarina: We could always loop back and add to .gitignore later, so maybe you can just delete this file for now so we can get this PR through. Thanks!

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 removed it in latest commit but held off on gitignore.

The reason I could see not having it in gitignore is because it's a signal you updated it manually last time. however if that's the case, we should rename the file to make it clear why it's being generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I might leave it out of .gitignore is that it is not an expected artifact of normal operations. As Sarina says, it's a bit of a warning. So I'd leave it more visible.

Copy link
Contributor

Choose a reason for hiding this comment

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

As-is, I think people will not understand what it is for and it will get committed and possibly merged. If you are thinking about additional checks around all this, something that failed the build if this file is found, with a clear message about why and how to fix would be useful. Again, as-is, I don't think you'll get what you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that simply letting the file get committed will not get what I want. I'm looking over possibilities for checking for the file automatically as a linting operation.

pylintrc Outdated Show resolved Hide resolved
@@ -287,7 +286,6 @@ disable =
illegal-waffle-usage,

logging-fstring-interpolation,
django-not-available,
Copy link
Contributor

Choose a reason for hiding this comment

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

This was the hand-edited line that is now missing from pylintrc, and is causing the linting failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah thanks, I'll add that to _tweaks

@sarina
Copy link
Contributor Author

sarina commented Sep 14, 2022

@nedbat I've seen this in another repo:

pylint_django:1:0: F5101: Django is not available on the PYTHONPATH (django-not-available)

I poked at this error and was not able to figure out what to do based on examples from cookiecutter and other repos

robrap
robrap previously approved these changes Sep 14, 2022
@robrap robrap dismissed their stale review September 14, 2022 16:37

Thought the last change got it to green.

@nedbat
Copy link
Contributor

nedbat commented Sep 14, 2022

You've added a line to pylintrc_tweaks, now you need to run edx_lint write pylintrc again to get the line into the pylintrc file.

@sarina
Copy link
Contributor Author

sarina commented Sep 14, 2022

uggggh i know that AND YET! I keep forgetting to do it.

@sarina
Copy link
Contributor Author

sarina commented Sep 14, 2022

Build is green, if someone wants to give me a final review (last commit actually applied the new pylintrc_tweaks to pylintrc)

@sarina sarina merged commit 7d48594 into master Sep 14, 2022
@sarina sarina deleted the tcril/fix-gh-org-url branch September 14, 2022 21:15
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