Perceptive Porcupines #6

janine9vn · 2021-07-30T17:44:04Z

No description provided.

This is just something to get us started, and most if not all of this will probably need to be changed as we start doing things.

Sorry i accidentally closed this instead of merging it

Fix flake8 errors

updated yaml files

Flit

Parsing stderr and identifying the key parts of the error.

Src: https://github.com/willmcgugan/textual/blob/main/examples/simple.py

Looks like textual doesn't accept params properly, they are getting over-wrriten in the package. Probably because it is in beta.

…tual Hook up Texttual

Stack Overflow API

hello world example

This will help explain what is happening in #89

Add command line option to clear the HTTP cache

…h-contribution Add installation and contribution instructions

…he-instructions Add instructions for clearning cache

Add gifs

Fallback to searching class name if no results

Also, a minified version of the Usage is shown if the arguments are not correct. This is usually the expected behaviour for command line applications unless `-h` or `--help` are explicitly passed in.

File not found

Refactor so

minor display updates

changed to version 0.1

updated readme for codejam submission

…9e4926f37ba2d' git-subtree-dir: perceptive-porcupines git-subtree-mainline: 6ef66e2 git-subtree-split: 022785f

HassanAbouelela

First Impressions - README

"All good projects start with the readme."
- Not an actual quote

I was very impressed by the readme. For end-users, it's short and straight to the point. The images were a nice touch!

I followed the contributing guide when setting up the project, and didn't run into any problems. That's usually a good sign you're doing something right. The contributing portion mentions all the details I'd need to contribute myself, big thumbs up from me.

One problem I have with the README is it's not clear what the relation between your project and the theme is, it would've been nice to touch on that briefly. The README is also a bit long. One thing that would've helped with that is breaking the contributing guide off into its own page (CONTRIBUTING.md is a common standard you may see in other open-source projects), and linking that in the README.

One more thing to note is that the image on your PyPI page is 404ing.

Use of Python and Rich/Textual

All green on pre-commit!

There is a very consistent use of docstrings and type annotations, making working with this project very nice. There were a few errors with annotations around the project, and I commented on them, but overall very minor. Nice job!

Rich is used nicely throughout the project, and the main display makes good use of textual.

The project is split up in a very logical way, and overall, it's pretty easy to follow.

Commit Quality

The quality of commits is pretty average. There are quite a few commits with too long a title, and quite a few with no messages, or unclear changes.

To address the first point, I personally have a hard limit of 72 characters for commit titles and messages, which allows them to display nicely in a timeline or list. Some people recommend 50 characters max for the title as well. If you're finding it difficult to give your commits short titles, remember that the title is just the most basic summary of all your changes. There isn't the same sort of limit for the bodies, so you can include all the other details there. If you are still finding it difficult to fit everything, could you possibly be packing too much into a single commit?

For the second point, make use of the commit's body to help future reviewers figure out why things are the way they are. They can help your peers and even yourself in the future.

Things you did well

This was an awesome project to review. It was clean, smart, and a really cool tool. These are a few things I believe were really well done:

The UX of the tool is really awesome. From the simple installation to the ease of use.
The code is very clean and easy to follow.
Caching the results is a smart decision, which I appreciate greatly.
The examples provided were a good way of getting familiar with the project.
The state of your repo is really neat. Having the issue templates is a great step towards future maintainability of the project. It's also nice to see you taking an active step towards maintaining the state of your license.

Things that could be improved

Here are a few things that I noticed while reviewing that could use some work:

Automatic publishing of the package in CI. This is really useful when it comes to having a healthy open source project in the future. A lot of tools make it pretty easy to do, including flit which can publish directly to PyPI.
General cleanliness. There are a few clean-up tasks that should be done. The placeholder code jam document needs to be removed, or possibly updated to mention this project originated from a code jam. If you end up updating the code jam document, it might be nice to link it in your readme, possibly under a title like "Our history".
Mypy. There seem to be the beginnings (or remenants) of a mypy setup. I see the config file, and I see a commit claiming to add it, but as far as I can tell, it was never added to the project dependencies and was never enforced in CI.

HassanAbouelela · 2021-08-13T16:54:09Z

perceptive-porcupines/wtpython/__main__.py

+            display_app_error(e)
+
+
+if __name__ == "__main__":


This is redundant. The name will never be anything other than main, unless someone explicitly imports this file (import wtpython.__main__). Since there doesn't seem to be a case where that's expected to happen, you don't need this check.

HassanAbouelela · 2021-08-13T16:58:47Z

perceptive-porcupines/wtpython/__main__.py

+    )
+
+
+def parse_arguments() -> dict:


I love how you've handled parsing. It's really clean and straightforward, and the product interface feels very intuitive. The additional information text is a nice touch.

HassanAbouelela · 2021-08-13T16:59:56Z

perceptive-porcupines/wtpython/__main__.py

+        parser.error("Please specify a script to run")
+        sys.exit(1)
+
+    if not os.path.isfile(opts['args'][0]):


I wouldn't have thought of doing this, but it makes for really nice UX. Well done!

HassanAbouelela · 2021-08-13T17:04:23Z

perceptive-porcupines/wtpython/__main__.py

+    return tb
+
+
+def run(args: list[str]) -> Exception:


The return type for this function should probably be marked as optional, since it'll return None if there is no exception.

HassanAbouelela · 2021-08-13T17:09:53Z

perceptive-porcupines/wtpython/settings.py

+SO_MAX_RESULTS = 10
+SO_API = "https://api.stackexchange.com/2.3"
+
+REQUEST_CACHE_LOCATION = Path.home() / Path(".wtpython_cache")


This is a sensible default, though it would've been nice if it could be configured or changed. Ultimately, it's not a big deal, just user preference.

We hope to have a config file setup, but I'm curious where you would choose to put the cache. I suspect /tmp might be a good spot as well.

This is a good place for most cases, it's just that as I'm running on windows, I'd probably place it with other caches in AppData.

That's not a bad idea either. Thanks!

HassanAbouelela · 2021-08-13T17:35:14Z

perceptive-porcupines/wtpython/stackoverflow.py

+        if clear_cache:
+            self.session.cache.clear()
+
+    def get_answers(self, question: dict) -> List[StackOverflowAnswer]:


This return type is a bit misleading. There are two main problems with it:

The code doesn't actually return a list. It returns a dict. The items key would return the list of answers.

The items key will return the list of answers, but not as instances of the StackOverflowAnswer (L62), but rather regular python dicts.

I believe this is just a mistake in the type hint, since you do in fact expect the full JSON result based on the way this output is used in StackOverflowQuestion. The return here should be marked as a dict, and if you want to clarify what a developer can expect to find in this dict, that should be added to the docstring.

HassanAbouelela · 2021-08-13T17:47:44Z

perceptive-porcupines/wtpython/__main__.py

+        so_results = so.search(error, SO_MAX_RESULTS)
+        if len(so_results) == 0:
+            # If no results have been found, search the error class name.
+            so_results = so.search(error.split(" ")[0].strip(":"), SO_MAX_RESULTS)


This is a pretty clever fallback!

I do want to note that when the fallback returns 0 hits, a special message is displayed in normal mode, but not no-display mode. I think it'll be neat if that gave a similar message.

HassanAbouelela · 2021-08-13T17:59:23Z

perceptive-porcupines/wtpython/__main__.py

+    traceback.print_exception(type(exc), exc, exc.__traceback__)
+    print(HorizontalRule())
+    print(
+        f":nerd_face: [bold][green]Please let us know by by opening a new issue at:[/] [blue underline]{GH_ISSUES}"


You may be interested in checking out hyperlinks. This goes for anywhere you use links. (Admittedly, they don't work on all terminals - including the one I tested on - but leaving in the visible link, as well as making it a hyper link would still work for everyone, and make it a bit easier for those that have it).

Example of what I mean:

Suggested change

f":nerd_face: [bold][green]Please let us know by by opening a new issue at:[/] [blue underline]{GH_ISSUES}"

f":nerd_face: [bold][green]Please let us know by by opening a new issue at:[/] [link={GH_ISSUES}][blue underline]{GH_ISSUES}[/link]"

HassanAbouelela · 2021-08-13T18:34:23Z

perceptive-porcupines/wtpython/display.py

+        questions: List[StackOverflowQuestion] = None,
+    ) -> None:
+        if questions is not None:
+            self.questions = questions


I don't believe the default value for questions is ever used, making it redundant.

If questions does end up as None though, the program will run into other errors as self.questions will not be defined.

Good catch. At the very least, it should be an empty list when passed.

HassanAbouelela · 2021-08-13T18:42:22Z

perceptive-porcupines/wtpython/display.py

+        footer.add_key("q", "Quit")
+        footer.add_key("←", "Prev Q")
+        footer.add_key("→", "Next Q")
+        footer.add_key("s", "Show Questions")


Since you're using a builtin widget, I'm not sure whether the following should be considered a bug or not, but here goes anyways:

Pressing show questions ends up hiding the footer. Clicking s again to go back leads to a broken footer. Here is an example:

Reproduction info:
GIF above is WSL on CMD, though I've tested and reproduced on powershell as well.

I'm pretty sure this is something from Textual, and is likely a windows specific issue since it's not really supported on Windows properly yet. I don't think any of us had this issue

That's fair!

This is a really minor problem, and overall, the experience is really smooth on windows.

ethansocal and others added 30 commits July 9, 2021 07:30

Initial commit

714a38f

Added some basic code layout

48cded2

This is just something to get us started, and most if not all of this will probably need to be changed as we start doing things.

Fixed capitalization

4cd307d

Fix more errors

16d5613

Merge pull request #5 from Perceptive-Porcupines/basic_layout

dc76c02

Sorry i accidentally closed this instead of merging it

updated yaml files

3a90d79

Changes that should hopefully fix the flake8 errors

8eee1ff

Merge branch 'basic_layout' into framework-update

8b27a75

Merge pull request #9 from Perceptive-Porcupines/basic_layout

348ff2d

Fix flake8 errors

Merge pull request #8 from Perceptive-Porcupines/framework-update

7f01621

updated yaml files

moved to flit framework

a189e57

updated readme

721e58e

Merge pull request #10 from Perceptive-Porcupines/flit

27e75a2

Flit

Spelling error

eec2fc1

Parsing stderr and identifying the key parts of the error.

ac30495

Fix for detecting errors

4e9cf91

Merge pull request #11 from Perceptive-Porcupines/interface

4acbf12

Parsing stderr and identifying the key parts of the error.

Spelling fix

9c374d6

Copy the example over from textual

c428f01

Src: https://github.com/willmcgugan/textual/blob/main/examples/simple.py

Fix pre-commit issues

286814b

Stack Overflow API

8bae3a1

Fixed

6026d36

Simplify the run statement on the main.py

f4f550b

Looks like textual doesn't accept params properly, they are getting over-wrriten in the package. Probably because it is in beta.

Rename Display window to pytui

c201cd5

Merge pull request #13 from Perceptive-Porcupines/feature/hook-up-tex…

ca4b418

…tual Hook up Texttual

Merge branch 'main' into Stack-Overflow-API

6ebe63f

minimal integration showing stack overflow result count

baa6942

Merge pull request #14 from Perceptive-Porcupines/Stack-Overflow-API

b5f6d71

Stack Overflow API

refactored to not have unecessary sub-packages

f6536b2

moved app settings to settings.py

1f6a470

ethansocal and others added 26 commits July 16, 2021 20:47

No Display gif

b125c98

fixed file not found error #83

a558f61

removed erroneous slash

8f122ca

Merge pull request #94 from what-the-python/examples

7a50510

hello world example

Fallback to searching class name if no results

0184dbc

Add installation and contribution instructions

49de8a4

Add instructions for clearning cache

79f22f5

This will help explain what is happening in #89

Merge pull request #89 from what-the-python/clear-cache

94325b7

Add command line option to clear the HTTP cache

Merge pull request #97 from what-the-python/feature/update-readme-wit…

87301b6

…h-contribution Add installation and contribution instructions

Merge pull request #98 from what-the-python/feature/add-clear-all-cac…

57961de

…he-instructions Add instructions for clearning cache

Merge pull request #92 from what-the-python/gifs

22a9ad4

Add gifs

Merge pull request #96 from what-the-python/better-search

d9b828e

Fallback to searching class name if no results

moved session generation and cache clear to constructor

63ada82

Add error message if script not specified

115571c

Also, a minified version of the Usage is shown if the arguments are not correct. This is usually the expected behaviour for command line applications unless `-h` or `--help` are explicitly passed in.

Merge branch 'main' into file-not-found

a622043

restructured search function

d06cf6a

Merge pull request #93 from what-the-python/file-not-found

2a7dd5e

File not found

Merge pull request #99 from what-the-python/refactor-so

f371695

Refactor so

minor display updates

c901aed

Merge pull request #100 from what-the-python/show-q-score

7a87d46

minor display updates

changed to version 0.1

f151b52

Merge pull request #101 from what-the-python/bump-version

6129cb4

changed to version 0.1

updated readme for codejam submission

86785cd

added hashtag

7dd5bf1

Merge pull request #105 from what-the-python/readme

022785f

updated readme for codejam submission

Add 'perceptive-porcupines/' from commit '022785fa710f3bf69becd0b2a42…

86d59e7

…9e4926f37ba2d' git-subtree-dir: perceptive-porcupines git-subtree-mainline: 6ef66e2 git-subtree-split: 022785f

HassanAbouelela reviewed Aug 13, 2021

View reviewed changes

This was referenced Aug 15, 2021

Provide more documentation for the SO filter and how it was created. VagishVela/wtpython#108

Closed

incorporated cj8 review comments VagishVela/wtpython#109

Merged

ethansocal mentioned this pull request Aug 24, 2021

Publish to PyPI workflow VagishVela/wtpython#130

Open

	f":nerd_face: [bold][green]Please let us know by by opening a new issue at:[/] [blue underline]{GH_ISSUES}"
	f":nerd_face: [bold][green]Please let us know by by opening a new issue at:[/] [link={GH_ISSUES}][blue underline]{GH_ISSUES}[/link]"

Uh oh!

Perceptive Porcupines #6

Are you sure you want to change the base?

Perceptive Porcupines #6

Uh oh!

Conversation

janine9vn commented Jul 30, 2021

Uh oh!

HassanAbouelela left a comment

Choose a reason for hiding this comment

First Impressions - README

Use of Python and Rich/Textual

Commit Quality

Things you did well

Things that could be improved

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants