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

Provide file names when "Failed to process {len(self.discovery.failures)} files" is returned #18

Closed
ryancheley opened this issue Jul 18, 2021 · 9 comments · Fixed by #25

Comments

@ryancheley
Copy link
Contributor

When running tryceratops if any of the files aren't able to process, more information should be returned about which files can't be processed.

Specifically, running tryceratops on a simple project with only 16 files returns the following:

Done processing! 🦖✨
Processed 16 files
Found 0 violations
Failed to process 1 files
Skipped 2340 files

Without knowing which file failed to process, I'm not sure what, if anything, I should do for next steps.

This message comes from interfaces.py line 51

@guilatrova
Copy link
Owner

Good point.
I was thinking about adding a -vv verbose flag to display all files. What do you think?

@ryancheley
Copy link
Contributor Author

I think that's a great idea! If you'd like I can try and make and update and submit a PR. It would be a couple of days though before I was able to get the PR submitted.

@ryancheley
Copy link
Contributor Author

@guilatrova I tried to get the project set up and installed locally in edit mode (pip install -e .) but I get the following errors:

ERROR: File "setup.py" not found. Directory cannot be installed in editable mode: /Users/ryan/github/tryceratops
(A "pyproject.toml" file was found, but editable mode currently requires a setup.py based build.)

What is the set up you use when developing locally. Perhaps I'm just missing something obvious.

Thanks!

@guilatrova
Copy link
Owner

@ryancheley Thanks for the help! I created a short new contributing doc, can you check and let me know if it works for you?

@ryancheley
Copy link
Contributor Author

@guilatrova I've made the changes and pushed them to my fork of the repo. Before I create a pull request, do you want to look at the changes I made? If so, you can see them here

If not, just let me know and I'll create the PR

Thanks!

@guilatrova
Copy link
Owner

That's a great start and thanks for following the conventions!

I think we can go even beyond though! I'd love to make your change so impactful that it can benefit other developers/contributors in the future.

How? Here:

Verbose means that any "verbose" (duh) message gets displayed in the console, besides the final user presentation (that we do with print), so for any developer today and in the future that wants to benefit from the flag you just implemented they should be able to do this ANYWHERE in Tryceratops and it should work.

I'd recommend then:

Extend logging for that

  • __main__.py -> Once you receive the flag, set the log level to DEBUG Reference
  • settings.py -> Create a new handler to output to console - what if we name this handle verbose_output? Feel free to name it as you prefer
  • settings.py -> Create a better format for the new DEBUG handler (Maybe lineno and asctime is too much. Try and share your decision!)

Do not use prints, do not bloat the presentation layer

  • interfaces.py -> Remove the prints/ifs from the presentation layer
  • discovery.py -> Use logging.debug inside the except block (should we remove the exception one? wdyt?)

Congratulations

🎉🥳 Now ANY DEV can use logging.debug ANYWHERE and it WORKS!

Overall

Does it make sense? I'm curious to receive your feedback. I'm not completely sure if it would work though, but I believe it worths trying.

I can help with if you're too busy, but I'd love to have your name as a contributor in this repository ❤️

@ryancheley
Copy link
Contributor Author

I really like the ideas above and can work on implementing something this week.

I'll need to look more closely at the try / except in discovery.py but based on my initial (very cursory) glance, I'd say it should be kept.

It seems like it's doing exactly what a try/except should be doing and I'm not aware of any other mechanisms to handle what it's doing (though maybe there is something?)

I'll continue to work on the branch of my fork and won't submit a PR for now.

Thanks!

P.S. I'd also love to have my name as a contributor on this project 😄

@guilatrova guilatrova moved this from To do to In progress in Tryceratops Roadmap 🦖🛣 Jul 21, 2021
@ryancheley
Copy link
Contributor Author

@guilatrova I think I have it figured out (also, I understand what you were asking about when you mentioned exception in your comment / question above) 🤦

I did swap out the logger.exception with logger.debug in discovery.py

My changes are located here.

There are still tests that need to be written for this change, but I wanted to get your input before starting to write the tests

Thanks!

@guilatrova
Copy link
Owner

Yes, you got the idea! Please open a PR so we can start discussing there ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants