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

[WIP]Added xonsh shell in dynamic #167

Closed

Conversation

sudojarvis
Copy link

Related Issue

Closes: #125

Describe the changes you've made

Added xonsh shell in dynamic.

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

Screenshots

Original Updated
original screenshot Screenshot from 2022-03-08 00-06-15use tab for further completion

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

🎉Congratulations!!🎉 for making your first PR , our mentors will review it soon.

@sudojarvis
Copy link
Author

@GouravSardana tests are failing. How can I resolve this?

@GouravSardana
Copy link
Member

@sudojarvis Wait I'll review it, Will try it locally and give you some suggestions

@GouravSardana
Copy link
Member

(temp) gourav@LAPTOP-PHS5P97J:~/ubuntu/dynamic-cli$ xonsh Traceback (most recent call last): File "/usr/bin/xonsh", line 3, in <module> from xonsh.main import main File "/home/gourav/ubuntu/dynamic-cli/xonsh/main.py", line 18, in <module> from xonsh.codecache import run_script_with_cache File "/home/gourav/ubuntu/dynamic-cli/xonsh/codecache.py", line 8, in <module> from xonsh.built_ins import XSH File "/home/gourav/ubuntu/dynamic-cli/xonsh/built_ins.py", line 24, in <module> from xonsh.ast import AST File "/home/gourav/ubuntu/dynamic-cli/xonsh/ast.py", line 120, in <module> from xonsh.built_ins import XSH ImportError: cannot import name 'XSH' from partially initialized module 'xonsh.built_ins' (most likely due to a circular import) (/home/gourav/ubuntu/dynamic-cli/xonsh/built_ins.py)

I pulled your PR locally but i couldn't activate the xonsh. Is there is something which I'm missing ?

@GouravSardana
Copy link
Member

cc @sudojarvis

@GouravSardana
Copy link
Member

Just to be sure - When i did "xonsh', It breaks

@GouravSardana
Copy link
Member

@sudojarvis I tested the changes, suggestion from my end:

  • It sometimes break with other configurations make sure all the version are stable
  • Do not use others repository code. Try to replicate the feature not the code.
  • I couldn't use dynamic as our common variable to activate the xonsh. I have to use xonsh for this.
  • We couldn't leverage the code which we have in the xonsh folder. Why? -> Because the thing which you showed as history autocomplete is not because of our code but it's just because of the xonsh. For testing -> Do not install dynamic and install just xonsh and type xonsh. You'll get all the functionalities which you showed. We are using xonsh to give our configuration to a single varible - "dynamic"
  • Lot's of bugs and security hotspot. So, Please work on that

Do not worry about issue level. I'll make this as level 4 or 5 once you'll understand the scope of this. I hope this make sense. Please let me know if you required anything else from our side.

cc @PradyumnaKrishna @pranavbaburaj

@sudojarvis
Copy link
Author

sudojarvis commented Mar 8, 2022

yup got it. will Try to do it again. And will try to add features to it.

@sonarcloud
Copy link

sonarcloud bot commented Mar 8, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug E 21 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 16 Security Hotspots
Code Smell A 533 Code Smells

No Coverage information No Coverage information
4.3% 4.3% Duplication

@sudojarvis sudojarvis closed this Mar 8, 2022
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.

Add xonsh shell in dynamic
2 participants