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

Various fixes #8

Merged
merged 1 commit into from Jan 12, 2021
Merged

Various fixes #8

merged 1 commit into from Jan 12, 2021

Conversation

NicolasCARPi
Copy link
Contributor

Hello,

I can see that this project is very young and there are a lot of things in the code that I think are suboptimal.

Please note: I didn't run any of the code yet!!! But for a few months now I had in mind to do exactly this kind of software, so I feel like contributing.

The main problem is that you assume too much, and take arbitrary decisions for the user, and this leads to software that is not portable. For instance, I'm using Archlinux, so your software will not work if I try to naively install it with install.sh, even if I have all the dependencies already in my $PATH.

The fixes are just minor stuff that I found while browsing the source, but I hope they will help you improve your skills ;)

Other things you might have:

  • use github actions to lint the code
  • use shellcheck on install.sh
  • use black and isort
  • add the licence and copyright in every file
  • add a CONTRIBUTING file
  • test the presence of dependencies before installing them
  • instead of git cloning directorysearch, consider adding it as a submodule
  • well there are many things you can do, but the first will be to review this PR ;)

Commit message below:

  • remove chmod +x instruction from README as it's not needed (git keeps
    the permissions)
  • don't use sudo to execute the install script as it contains sudo
    commands
  • link to the MIT license in the README
  • replace tabs with spaces
  • remove trailing whitespaces
  • add newline at end of files

In install.sh:

  • use a variable to hold the dependencies instead of repeating them
  • use the --user flag for pip install
  • use /usr/local/bin instead of /usr/bin for symlink
  • store dirsearch in ~/.opt instead of /opt: don't pollute the system
    with user software, don't use root to clone a git repository
  • use $() instead of backquotes
  • add quotes for variables
  • use env to find bash

* remove chmod +x instruction from README as it's not needed (git keeps
  the permissions)
* don't use `sudo` to execute the install script as it contains `sudo`
  commands
* link to the MIT license in the README
* replace tabs with spaces
* remove trailing whitespaces
* add newline at end of files

In install.sh:

* use a variable to hold the dependencies instead of repeating them
* use the `--user` flag for pip install
* use /usr/local/bin instead of /usr/bin for symlink
* store dirsearch in ~/.opt instead of /opt: don't pollute the system
  with user software, don't use root to clone a git repository
* use `$()` instead of backquotes
* add quotes for variables
* use env to find bash
@Anteste
Copy link
Owner

Anteste commented Jan 12, 2021

Hi Nicolas,
Thank you for submiting this pull request you did an awesome job but I have some questions .

  • Where do I have to put the License in a comment on the header of every file ?
  • How can I lint the code ?

@Anteste Anteste merged commit 12a4139 into Anteste:1.x Jan 12, 2021
@Anteste Anteste added the Bug Something isn't working label Jan 12, 2021
@NicolasCARPi
Copy link
Contributor Author

Where do I have to put the License in a comment on the header of every file ?

I see you did it already.

How can I lint the code ?

Look into pylint, look into black maybe as a git pre commit hook, and isort to sort the imports.

Also, try and use Pathlib instead of the old os module. Pathlib is great! And you're already using f-strings, so why not continue on the road of modern python 3 ;)

@NicolasCARPi NicolasCARPi deleted the fixes branch January 12, 2021 09:13
@Anteste
Copy link
Owner

Anteste commented Jan 12, 2021

Also, try and use Pathlib instead of the old os module. Pathlib is great!

Where can I use the pathlib module on this script. because but I can't execute commands using the pathlib module, the subprocess module is better in this case.

@NicolasCARPi
Copy link
Contributor Author

For instance in createDir of conf.py. Pathlib is for paths, not commands ;) Makes your life easier, trust me!

Also the reOpen() function should be corrected with the new path (or maybe use a better way to check for it, without a hardcoded path but look if the command is in the $PATH).

@Anteste
Copy link
Owner

Anteste commented Jan 12, 2021

Thank you Nicolas I will try to do that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants