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

Stage 030 update #73

Merged
merged 2 commits into from
Dec 5, 2017
Merged

Stage 030 update #73

merged 2 commits into from
Dec 5, 2017

Conversation

Evildoor
Copy link
Contributor

@Evildoor Evildoor commented Nov 17, 2017

Small update with changes serving for:

  • Catching up with dataflow-with-document-content -> master migration.
  • Creating a base for extension development. (Removed per conversation below.)

Copy link
Collaborator

@mgolosova mgolosova left a comment

Choose a reason for hiding this comment

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

I don`t see how changes from e3c85e6 (CSV file with "hashtags" and "physics short" for datasets) are used.
I believe that this commit would be better for another branch, where mentioned "extensions" do appear.
Two others will be accepted as they are. Thank you!

But, for the future:

  • it is a good idea to describe errors that was fixed, so that one who stumbled on it knew it was fixed (5fa3369);
  • same for the improved regular expression (6c3d04b): what changed after this improvements? what cases it failed to detect (or detected erroneously) before so that this improvement is needed?

It is good at least because when you yourself try to reconstruct your thoughts a year after now, you might already forget what is obvious for now. No to mention people who tries to understand your thoughts right now ;)

@Evildoor
Copy link
Contributor Author

CSV file - i added it because i thought that it was better than either:

  1. Making a commit which does nothing besides creating a directory.
  2. Not making a commit, and then creating this directory in each branch which adds an extension.

Error - makes sense, will try to keep that in mind.

Regular expression - this change is solving #46. When this pull request will be merged, issue will be closed and commit/request will be linked in it. I can link issue here or in pull request initial message, but not in commit message (i think).

@mgolosova
Copy link
Collaborator

@Evildoor,
I see the problem.
For Git itself, there`s nothing wrong in same dir created in different branches -- as it is interested mostly in files, to create or delete a directory is kind of a "side-effect" of adding/removing data from it. Git would only worry if you add different files with the same name, and then try to merge them.

However if there are too many branches, and you`d like to avoid mkdir for each of them, I would recomend to create a branch like "stage-30-extensions", create empty dir [1] (or put there all the source data you are gonna need for the extensions), and then create branches for individual extensions from that one.
Then even if you create PR from that branches directly to the master branch, the "create empty dir" commit will disappear from all the PRs as soon as the first one is merged.

[1] As Git is not interested in any directory unless it has at least one tracked file, to add an empty dir is a tricky thing. But you can add something like README file to it, where you are going to make notes about "which data are for which extension" (if it is a directory for source data only) or describing extensions themselves (if their code to be stored in it as well), or something.

@Evildoor
Copy link
Contributor Author

Evildoor commented Nov 28, 2017

I doubt that there will be too many branches with different extensions, but idea of creating "stage-30-extensions" branch looks appealing to me. Will do.

This solves a problem described in Issues, link will be made.
Starting to add papers and pressing "Cancel" in the dialogue lead to
"fnames" variable being not what the program expected. Check was added
to prevent this.
@Evildoor
Copy link
Contributor Author

Evildoor commented Dec 1, 2017

Removed commit related to extensions, updated descriptions for other ones.

@mgolosova
Copy link
Collaborator

@Evildoor,
Thank you, I think the PR can be merged now.
Just ine question: am I right and the 862e7cb is related to the issue #14? I`d like to mention it in the merge commit at least, as in console no one will see cross-links that will possibly appear later in the web-interface ;-)

@Evildoor
Copy link
Contributor Author

Evildoor commented Dec 4, 2017

@mgolosova
Good.
Uh, no, 862e7cb is related to #46, not #14.

@mgolosova
Copy link
Collaborator

@Evildoor,
right, thank you. Was not that obvious ;)

@mgolosova mgolosova merged commit 070fdf0 into master Dec 5, 2017
@mgolosova
Copy link
Collaborator

@Evildoor ,
request merged, thank you. Please remove the branch if it is no longer needed.

@Evildoor Evildoor deleted the stage-030-update branch December 5, 2017 12:14
@Evildoor Evildoor mentioned this pull request Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants