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

qucs#956 command line #974

Closed
wants to merge 23 commits into from
Closed

qucs#956 command line #974

wants to merge 23 commits into from

Conversation

oxmon-2500
Copy link
Contributor

  • qucs without arguments working as previously
  • qucs with arguments see usage
    The following statements in qucs.cpp do not work as supposed (no any action):
    //-------------------------------------------------------------------------------------------------------
    // select (highlight) project in QListView (projects)
    QModelIndex idx = m_homeDirModel->index(openProjName, 0); //FIXME
    Projects->selectionModel()->select(idx, QItemSelectionModel::Select); //FIXME
    //-------------------------------------------------------------------------------------------------------
    Possible someone with Qt-experience can solve it in the future

@in3otd
Copy link
Contributor

in3otd commented Feb 22, 2020

there are too many, apparently unrelated, changes and it's difficult to readily understand what are the actual functional changes.

Whitespace/indentation changes in code that did not need to be touched otherwise should not be done, see the coding conventions in the Contributing guidelines.
If you really, really, really need to change the existing whitespace (hint: you usually don't), those changes should go in a separate commit.

@oxmon-2500
Copy link
Contributor Author

there are too many, apparently unrelated, changes and it's difficult to readily understand what are the actual functional changes.

Whitespace/indentation changes in code that did not need to be touched otherwise should not be done, see the coding conventions in the Contributing guidelines.
If you really, really, really need to change the existing whitespace (hint: you usually don't), those changes should go in a separate commit.

The problem lies in not updated tests module SchematicTests.cpp and I hope the next travis run will be OK.
Regarding white space I deactivate spaces-compare in diff-programs, but you are right to make as few as possible unrelated changes.

@in3otd
Copy link
Contributor

in3otd commented Feb 22, 2020

I'll let you fix the Travis error; I have already split your initial commit in a few parts, separating the whitespace changes (which will be removed). Once CI is passing I'll push these changes on top of your commits, so you can take a look. Later I'll squash the unneeded commits.
I did a quick test and the added option seems to work as expected also here.

@in3otd
Copy link
Contributor

in3otd commented Feb 22, 2020

Note that OSX has a new error, independent of the code from this PR:
see line 99 here or below.
As a consequence Qt4 is not installed and the configuration fails.
@guitorri ?

$ rvm $brew_ruby do brew bundle --verbose --global
/usr/local/bin/brew tap homebrew/bundle
==> Tapping homebrew/bundle
Cloning into '/usr/local/Homebrew/Library/Taps/homebrew/homebrew-bundle'...
Tapped (102 files, 250.9KB).
Error: Unknown command: bundle

@in3otd
Copy link
Contributor

in3otd commented Feb 23, 2020

ah, thanks - my google-fu was weak apparently 😁.
I've opened #975 to fix that.

by specifiying the project path and optionally
the name of contained schematics that should be
loaded:

qucs [ projectPath_prj [ /schematicName.sch ] ]
when creating the components list from -list-entries
@in3otd
Copy link
Contributor

in3otd commented Feb 23, 2020

I've split your changes, grouping them broadly according to their purpose. There are a few reverts just to avoid force-pushing, for the moment. Later this will be rebased on the latest develop and those reverts squashed/removed.

I see that after all the changes, I now appear to be the author of all the commits, I'll fix that when rebasing.

@guitorri
Copy link
Member

Please rebase and cleanup history.
One commit for your feature, another for the whitespace (if you must).

@oxmon-2500
Copy link
Contributor Author

Please rebase and cleanup history.
One commit for your feature, another for the whitespace (if you must).

Hi, after checks are OK I am going to rebase:
git rebase -i 5c696ce (the first commit in the feature branch) Makes it also cleanup history, or is there an another git command?

oxmon-2500 and others added 2 commits March 1, 2020 06:27
#956 command line tests

#956 command line tests-2

travis_qt_test_01

Revert all previous changes back to `develop`

This reverts commit 5c696ce.

Allow opening a project from the command line

by specifiying the project path and optionally
the name of contained schematics that should be
loaded:

qucs [ projectPath_prj [ /schematicName.sch ] ]

Specify explicitly categories to skip

when creating the components list from -list-entries

Use enums to refer to Project View tabs

Whitespace changes only

Revert "Whitespace changes only"

This reverts commit ff1100b.

travis_qt_test_04

Temporary fix for Travis OSX issue
If a Diagram contained a Graph which referenced a variable in a
Dataset with a name containing spaces, the Graph loading failed
as the simple parsing code was confused by the spaces in the
Dataset name.
Rmano and others added 3 commits March 1, 2020 06:28
The linblocks tutorials teach how to use QUCS to simulate the frequency
response of linear blocks, and how to define them using the subcircuit
feature.
#956 command line qucs.cpp .isNull

#956 command line .travis fi
@guitorri
Copy link
Member

guitorri commented Mar 1, 2020

It is good practice to update develop and rebase on top of it.

Something like:

git checkout develop
git pull [origin] develop #origin is the project remote
git checkout [feature]
git rebase develop # put your [feature] branch on top of develop.

@in3otd
Copy link
Contributor

in3otd commented Mar 1, 2020

I'd suggest to use -i with rebase so that you can choose whether to squash/skip some commits

@oxmon-2500
Copy link
Contributor Author

oxmon-2500 commented Mar 1, 2020

I'd suggest to use -i with rebase so that you can choose whether to squash/skip some commits

Its already done: git rebase -i e93a709
all lines changed to squash, except:
pick 5c696ce #956 command line (first line)
pick d5f9d79 Fix Graph loading when Dataset names contain spaces
pick cb15cd0 Add a tutorial on lineal blocks and frequency response
I hope this is OK, if not I will delete this PR and make a new one with only one commit.

By the way, I am creating a git-wiki-page with the workflow step-by-step:
https://github.com/oxmon-2500/qucs_wiki/wiki
Any suggestion to change/expand it would be appreciated
Cheers

@oxmon-2500
Copy link
Contributor Author

A new PR#981 Qucs 956 command line clean replaces this one

@in3otd
Copy link
Contributor

in3otd commented Mar 2, 2020

in general there is no need to open a new PR if you are working on the same changes; just push --force your changes to the same branch.

@guitorri
Copy link
Member

guitorri commented Mar 3, 2020

@oxmon-2500, I see you are struggling with rebase.
Before doing a PR, get used to update develop from upstream and rebase your branch with respect to the latest develop.
Ideally the PR should contain only your fixes, not commits that are already merged into develop.
Right now you are using a very old commit to rebase. This way the rebase will pick up commits in between that have nothing to to do with your PR.
Whenever possible work based on the latest develop.

I typically use the following:

git checkout develop
git pull [origin] develop   #origin is the project remote
git checkout [feature]
git rebase develop  # put your [feature] branch on top of develop
git checkout -b feature-cleanup  # new branch for cleanup operation
git rebase -i develop  # iterative rebase cleanup the mess, keep original branch just in case
git push -f guitorri feature-cleanup:feature # overwrite branch on existing PR

@oxmon-2500 oxmon-2500 closed this Mar 10, 2020
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