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

update the constant name and imports #4

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

Conversation

ai-naymul
Copy link

Constants name is transformed into all uppercase and remove an unimportant comment in import and removed some unimportant imports in urllib module.

Constants name is transformed into all uppercase and remove an unimportant comment in import and removed some unimportant imports in urllib module
.idea/.gitignore Outdated Show resolved Hide resolved
@unhammer
Copy link
Member

unhammer commented Sep 4, 2023

I can't quite see what this change achieves, @jonorthwash I think you'll have to decide if it's something we want

@ai-naymul
Copy link
Author

I can't quite see what this change achieves, @jonorthwash I think you'll have to decide if it's something we want

would you like to add Multiprocessing or Multithreading to speed up the extraction peocess or Logging functionality in the project?

@TinoDidriksen
Copy link
Member

@ai-naymul, you accidentally inverted the deletion in the commit. You kept all the files that we wanted gone, and removed the one file that had real changes.

@ai-naymul
Copy link
Author

@ai-naymul, you accidentally inverted the deletion in the commit. You kept all the files that we wanted gone, and removed the one file that had real changes.

@TinoDidriksen I am extremely sorry for that, What should I do now, could you please suggest me?

@TinoDidriksen
Copy link
Member

Roll back your commit and remove the other files.

@ai-naymul
Copy link
Author

Roll back your commit and remove the other files.

@TinoDidriksen How can I do that could you please help me or should I made another pr?

@TinoDidriksen
Copy link
Member

Undo commit 547c64244d5c985e96a40f0becec3eb054b1a7e2 and remove the .idea folder instead.

@ai-naymul
Copy link
Author

ai-naymul commented Sep 5, 2023

Undo commit 547c64244d5c985e96a40f0becec3eb054b1a7e2 and remove the .idea folder instead.

@TinoDidriksen You mean I should revert the commit remove all those .idea files...?

Screenshot_1

Like that...

Screenshot_2

@TinoDidriksen
Copy link
Member

Something like that, yes. The changeset should only be changes to WikiExtractor.py - we don't want any generated files added, especially not IDE-specific ones.

@ai-naymul
Copy link
Author

Something like that, yes. The changeset should only be changes to WikiExtractor.py - we don't want any generated files added, especially not IDE-specific ones.

Okay then let me make the revert, Thanks a lot for your help❤

@ai-naymul
Copy link
Author

Something like that, yes. The changeset should only be changes to WikiExtractor.py - we don't want any generated files added, especially not IDE-specific ones.

I have made an another commit could you please check that give me feedback?

@TinoDidriksen
Copy link
Member

You didn't revert everything from the commit. You need to go back to commit ai-naymul@0f313a9 and remove the .idea folder from that state.

@ai-naymul
Copy link
Author

You didn't revert everything from the commit. You need to go back to commit ai-naymul@0f313a9 and remove the .idea folder from that state.

Should I make another commit in this regarding there are some error is troughing while I tried to delete the whole idea file?

@mr-martian
Copy link

@ai-naymul As it stands now, your pull request makes literally 0 changes. Additionally, it's still not clear what was gained by the original change. What is the value of renaming those variables and very slightly reformatting the import lines?

@TinoDidriksen
Copy link
Member

Whether you add a 4th commit with the actual changes, or rewrite history to remove some commits and then remove the folder, is really up to you. If we merge, we'll squash it in so it looks like a single commit anyway.

@ai-naymul
Copy link
Author

@ai-naymul As it stands now, your pull request makes literally 0 changes. Additionally, it's still not clear what was gained by the original change. What is the value of renaming those variables and very slightly reformatting the import lines?

It helps to read the code better but however If think It's not worthy you can closed the PR and if you like I would love to work on another things to improve the project :)

@TinoDidriksen
Copy link
Member

No, you misunderstand. Right now, this PR literally does nothing. There are no changes in https://github.com/apertium/WikiExtractor/pull/4/files - you've managed to remove everything, not just the .idea folder.

@ai-naymul
Copy link
Author

No, you misunderstand. Right now, this PR literally does nothing. There are no changes in https://github.com/apertium/WikiExtractor/pull/4/files - you've managed to remove everything, not just the .idea folder.

What should I do now?

@TinoDidriksen
Copy link
Member

I repeat: You need to go back to commit ai-naymul@0f313a9 and remove the .idea folder from that state.

@ai-naymul
Copy link
Author

I repeat: You need to go back to commit ai-naymul@0f313a9 and remove the .idea folder from that state.

Bro I mean how can I delete the folder from that commit could you please give me some guide to make that, I will be very gratefull for that... :)

@TinoDidriksen
Copy link
Member

Something like:

git reset --hard 0f313a9dbd59c8a2d1d25e61f10e2ffbf5989290
rm -rf .idea
git commit --all -m "Removed folder"
git push -f

@ai-naymul ai-naymul force-pushed the fix/constantname_imports branch from 653508d to 59b0bbb Compare September 7, 2023 11:09
@ai-naymul
Copy link
Author

Something like:

git reset --hard 0f313a9dbd59c8a2d1d25e61f10e2ffbf5989290
rm -rf .idea
git commit --all -m "Removed folder"
git push -f

@TinoDidriksen Is that fine now?

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.

4 participants